HADOOP-19189. ITestS3ACommitterFactory failing #6857
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* parameterize the test run rather than do it from within the test suite. * log what the committer factory is up to (and improve its logging) * close all filesystems, then create the test filesystem with cache enabled. The cache is critical, we want the fs from cache to be used when querying filesystem properties, rather than one created from the committer jobconf, which will have the same options as the task committer, so not actually validate the override logic. The tests were therefore failing correctly, because the test environment wasn't being set up. I have no idea why this worked before. Change-Id: I69e989c8c0a8d9ac3bd2a4a34512cad94b579ba9
|
🎊 +1 overall
This message was automatically generated. |
Change-Id: I77fddee6289230a0f99eb25b8621d6c1fd390cb5
|
🎊 +1 overall
This message was automatically generated. |
|
Seeking feedback from @mukund-thakur @ahmarsuhail @HarshitGupta11 @virajjasani Fixes test failures, nothing else... |
| filesystemConfRef.unset(FS_S3A_COMMITTER_NAME); | ||
| assertFactoryCreatesExpectedCommitter(FileOutputCommitter.class); | ||
| } | ||
| private final String fsCommitterName; |
...tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/commit/ITestS3ACommitterFactory.java
Show resolved
Hide resolved
| assertFactoryCreatesExpectedCommitter(FileOutputCommitter.class); | ||
| } | ||
| private final String fsCommitterName; | ||
| private final String pathCommitterName; |
There was a problem hiding this comment.
why do we need this? Isn't just first one enough
Change-Id: I5a233329b7bae9da9b894f5226f20437bf7672fc
|
updated version; tested s3 london |
mukund-thakur
approved these changes
Jun 6, 2024
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
reviewed
Jun 7, 2024
Comment on lines
199
to
201
| LOG.info("Filesystem Committer='{}'; task='{}'", | ||
| fsConf.get(FS_S3A_COMMITTER_NAME), | ||
| jobConf.get(FS_S3A_COMMITTER_NAME)); |
There was a problem hiding this comment.
we can also log description here for better debugging?
There was a problem hiding this comment.
this is the thread name so log prints it anyway, but I've just checked and as this is a setup operation, it is just "setup", though the next log entry is complete
2024-06-07 14:20:59,136 [setup] INFO commit.ITestS3ACommitterFactory (ITestS3ACommitterFactory.java:setup(199)) - Filesystem Committer='null'; task='null'
2024-06-07 14:20:59,136 [JUnit-testBinding[Default Binding-fs=[]-task=[]-[class org.apache.hadoop.mapreduce.lib.output.FileOutputCommitter]]] INFO s3a.AbstractS3ATestBase (AbstractS3ATestBase.java:describe(227)) -
adding it to this log message
...tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/commit/ITestS3ACommitterFactory.java
Outdated
Show resolved
Hide resolved
Change-Id: Iaf8263813940597b8b7d8da0db991018a2f35ca7
virajjasani
approved these changes
Jun 7, 2024
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
added a commit
that referenced
this pull request
Jun 7, 2024
* parameterize the test run rather than do it from within the test suite. * log what the committer factory is up to (and improve its logging) * close all filesystems, then create the test filesystem with cache enabled. The cache is critical, we want the fs from cache to be used when querying filesystem properties, rather than one created from the committer jobconf, which will have the same options as the task committer, so not actually validate the override logic. Contributed by Steve Loughran
KeeProMise
pushed a commit
to KeeProMise/hadoop
that referenced
this pull request
Sep 9, 2024
* parameterize the test run rather than do it from within the test suite. * log what the committer factory is up to (and improve its logging) * close all filesystems, then create the test filesystem with cache enabled. The cache is critical, we want the fs from cache to be used when querying filesystem properties, rather than one created from the committer jobconf, which will have the same options as the task committer, so not actually validate the override logic. Contributed by Steve Loughran
Hexiaoqiao
pushed a commit
to Hexiaoqiao/hadoop
that referenced
this pull request
Sep 12, 2024
* parameterize the test run rather than do it from within the test suite. * log what the committer factory is up to (and improve its logging) * close all filesystems, then create the test filesystem with cache enabled. The cache is critical, we want the fs from cache to be used when querying filesystem properties, rather than one created from the committer jobconf, which will have the same options as the task committer, so not actually validate the override logic. Contributed by Steve Loughran
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The cache is critical, we want the fs from cache to be used when querying filesystem properties, rather than one created from the committer jobconf, which will have the same options as the task committer, so not actually validate the override logic.
The tests were therefore failing correctly, because the test environment wasn't being set up.
I have no idea why this worked before.
How was this patch tested?
Reworked ITestS3ACommitterFactory tested against s3 london.
rerunning full -Dscale test to make sure that none of the changes i made to the production code have problems
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?