Skip to content

HADOOP-16134 001- initial design of a WriteOperationsContext - #515

Closed
steveloughran wants to merge 1 commit into
apache:trunkfrom
steveloughran:s3/HADOOP-16134-WriteOpContext
Closed

HADOOP-16134 001- initial design of a WriteOperationsContext#515
steveloughran wants to merge 1 commit into
apache:trunkfrom
steveloughran:s3/HADOOP-16134-WriteOpContext

Conversation

@steveloughran

Copy link
Copy Markdown
Contributor

Does not compile

This adds

  • a context which is passed round with writes
  • a parent delete policy as part of this (unused)

This PoC shows that adding a new context everywhere is overcomplex as you now need to retrofit it through the stack, even though a (single, shared) WriteOperationsHelper is already passed in

This doesn't compile: I put it together while half-listening to an online talk, and now I've done I've learned enough to say "not the right approach"

Better strategy:

  • include the WriteOperationsContext in the WriteOperationsHelper; instantiating a new one each time. This will automatically add it to all bits of the FS code which write data
  • add a default/configurable delete policy to the FS, but allow operations to explicitly overwrite this. Example: completing all the committed work in a job commit, because we can rely on the write of the _SUCCESS file to do the work (so only do it for one file, not every file created)

We're also a bit constrained by how the MPU API of HADOOP-13186 tries to be independent of the FS instance -this is one of those cases where it complicates life even more. The FS/FC MUST be the factory for MPU instances.

Change-Id: I0de1d4b97fdf4c4f0ece1a27245ba9bb38a29559

This adds
* a context which is passed round with writes
* a parent delete policy as part of this (unused)
This PoC shows that adding a new context everywhere is overcomplex as you now need to retrofit it through the stack, even though a (single, shared) WriteOperationsHelper is already passed in
Better strategy:
* include the WriteOperationsContext in the WriteOperationsHelper; instantiating a new one each time
* add a default/configurable delete policy to the FS, *but allow operations to explicitly overwrite this*. Example: completing all the committed work in a job commit, because we can rely on the write of the _SUCCESS file to do the work (so only do it for one file, not every file created)
Change-Id: I0de1d4b97fdf4c4f0ece1a27245ba9bb38a29559
private final PutTracker putTracker;

private final S3AWriteOpContext writeContext;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace:end of line

instrumentation.newOutputStreamStatistics(statistics),
getWriteOperationHelper());
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace:end of line

status = null;
}

final String key = pathToKey(dst);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace:end of line

}


}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace:end of line

@apache-yetus

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
0reexec29Docker mode activated.
_ Prechecks _
+1@author0The patch does not contain any @author tags.
+1test4tests0The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1mvninstall1177trunk passed
+1compile29trunk passed
+1checkstyle25trunk passed
+1mvnsite37trunk passed
+1shadedclient682branch has no errors when building and testing our client artifacts.
+1findbugs43trunk passed
+1javadoc22trunk passed
_ Patch Compile Tests _
-1mvninstall21hadoop-aws in the patch failed.
-1compile19hadoop-aws in the patch failed.
-1javac19hadoop-aws in the patch failed.
-0checkstyle16hadoop-tools/hadoop-aws: The patch generated 2 new + 5 unchanged - 1 fixed = 7 total (was 6)
-1mvnsite20hadoop-aws in the patch failed.
-1whitespace0The patch has 4 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
+1shadedclient670patch has no errors when building and testing our client artifacts.
-1findbugs22hadoop-aws in the patch failed.
+1javadoc19the patch passed
_ Other Tests _
-1unit23hadoop-aws in the patch failed.
+1asflicense27The patch does not generate ASF License warnings.
2942
SubsystemReport/Notes
DockerClient=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/artifact/out/Dockerfile
GITHUB PR#515
Optional Testsdupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
unameLinux 40849de35f4f 4.4.0-138-generic #164-Ubuntu SMP Tue Oct 2 17:16:02 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitypersonality/hadoop.sh
git revisiontrunk / 632d5e8
mavenversion: Apache Maven 3.3.9
Default Java1.8.0_191
findbugsv3.1.0-RC1
mvninstallhttps://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/artifact/out/patch-mvninstall-hadoop-tools_hadoop-aws.txt
compilehttps://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/artifact/out/patch-compile-hadoop-tools_hadoop-aws.txt
javachttps://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/artifact/out/patch-compile-hadoop-tools_hadoop-aws.txt
checkstylehttps://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/artifact/out/diff-checkstyle-hadoop-tools_hadoop-aws.txt
mvnsitehttps://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/artifact/out/patch-mvnsite-hadoop-tools_hadoop-aws.txt
whitespacehttps://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/artifact/out/whitespace-eol.txt
findbugshttps://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/artifact/out/patch-findbugs-hadoop-tools_hadoop-aws.txt
unithttps://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/artifact/out/patch-unit-hadoop-tools_hadoop-aws.txt
Test Resultshttps://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/testReport/
Max. process+thread count446 (vs. ulimit of 5500)
modulesC: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
Console outputhttps://builds.apache.org/job/hadoop-multibranch/job/PR-515/1/console
Powered byApache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

@steveloughran

Copy link
Copy Markdown
ContributorAuthor

closing this for now; doing a different refactoring

shanthoosh added a commit to shanthoosh/hadoop that referenced this pull request Oct 15, 2019
**Changes:**
* Enable all existing standalone integration tests except `TestZkStreamProcessorSession`(`TestZkStreamProcessorSession` is flaky. It spawns `x` StreamProcessors and kills one StreamProcessor through zookeeper session expiration. Sleeps for 5 seconds and proceeds to do validation. If the rebalancing phase takes longer the sleep time, validation fails).
* Remove zookeeper unavailable unit test from LocalApplicationRunner(Race condition in zookeeper shutdown fails other tests). The deleted test will be added back in a separate test class.
* Increase zookeeper server minimum session timeout from 6 seconds to 120 seconds.
* Add assertions to validate if kafka topics setup were successful before the unit tests.
**Validation:**
Verified by running the following script on top of this patch in master branch.
```bash
i=0
while [ $i -lt 50 ]; do
i=`expr $i + 1`
echo "Run " +$i
./gradlew clean :samza-test:test -Dtest.single="TestZkLocalApplicationRunner" --debug --stacktrace >> ~/test-logs_10
./gradlew clean :samza-test:test -Dtest.single="TestZkStreamProcessor" --debug --stacktrace >> ~/test-logs_10
./gradlew clean :samza-test:test -Dtest.single="TestStreamProcessor" --debug --stacktrace >> ~/test-logs_10
./gradlew clean :samza-test:test -Dtest.single="TestZkStreamProcessorFailures" --debug --stacktrace >> ~/test-logs_10
done;
```
**Result:**
```bash
[svenkatasvenkata-ld2 samza]$ grep 'BUILD SUCCESS' ~/test-logs_10 | wc -l
200
[svenkatasvenkata-ld2 samza]$ grep 'BUILD FAIL' ~/test-logs_10 | wc -l
0
```
Author: Shanthoosh Venkataraman <santhoshvenkat1988@gmail.com>
Author: Shanthoosh Venkataraman <svenkataraman@linkedin.com>
Reviewers: Xinyu Liu <xinyu@apache.org>
Closesapache#515 from shanthoosh/turn_all_integration_tests_on
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@steveloughran@apache-yetus