-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-16383. Pass ITtlTimeProvider instance in initialize method in … #1009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-16383. Pass ITtlTimeProvider instance in initialize method in … #1009
Conversation
|
Tests run against ireland. Got a few errors in the sequential tests: I don't think that those I related, and will go away if I clear the bucket and use a fresh ddb table. I'll check tomorrow on what can be the cause on this. |
|
Yes, org.apache.hadoop.fs.contract.s3a.ITestS3AContractRootDir is failing even without my patch on trunk! It would worth looking into it. |
|
🎊 +1 overall
This message was automatically generated. |
...p-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStore.java
Outdated
Show resolved
Hide resolved
...p-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStore.java
Outdated
Show resolved
Hide resolved
...p-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStore.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line width
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*I've made the FileSystem TTL Provider one of the attributes you can get from the FS via a StoreContext
bindToOwnerFilesystem() is picking this up, looks like all that is needed is to delete the timeProvider = new S3Guard.TtlTimeProvider(conf); line for the owner binding to be picked up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline with Gabor. Outcome of that conversation: bindToOwnerFileSystem doesn't exist everywhere and there isn't already a context created outside of the context (ha!) of certain operations. But we should have a context created earlier since it doesn't contain state that changes between operations (I actually wonder why we're creating a new instance for every operation instead of the metadatastore getting a permanent context). We need to check the context is complete enough, as this is called during FS initialization, precisely when the createStoreContext() javadoc warns you to be careful :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should avoid that for now, and use it during another refactor maybe. I've fixed all the other stuff with my latest commit. Excluding this would change the whole point of this jira/pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Just be aware, I consider it transient.
@mackrorysd -as to why the context changes: it includes the metastore, so once that is created things changed. But yes, everything in init is brittle; including stuff like setting up delegation tokens (needed before you make any S3 calls)
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/LocalMetadataStore.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/LocalMetadataStore.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See discussion in DynamoDBMetadataStore.initialize() about why we don't need an explicit entry here
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/MetadataStore.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/MetadataStore.java
Outdated
Show resolved
Hide resolved
...-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/impl/TestPartialDeleteFailures.java
Outdated
Show resolved
Hide resolved
|
Review summary
The main arguments in favour of that explicit binding to a TTL provider are
If you look at the slow refactoring I've started in HADOOP-15183, you can see that I've been pushing stuff into StoreContext, with the ultimate goal of all subsidiary parts of the s3a codebase (metastore, delegation tokens, S3ABlockOutputStream, etc) to not get given an S3AFS instance any more, just a StoreContext with access to lower level operations through binding callbacks (WriteOperationHelper, etc). Why so: it'll line us up for actually splitting up S3AFS into layers What does that mean now, well, not much: we can cast to S3AFS and extract the TTL, or take it as an argument as this patch does. I don't see it being that significant either way, except ultimately, I do hope to replace that initialize(FileSystem) with an Initialize(StoreContext), and, as that serves up the TTL, it'd be the consistent way to get it |
e551a7c to
c6cca7e
Compare
|
rebased to trunk |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Test run against ireland: AbstractITCommitMRJob.testMRJob is known, stacktrace for deletetable: |
c6cca7e to
b7bbdb6
Compare
…MetadataStore interface Fix things based on review and checkstyle issues
b7bbdb6 to
32e51b0
Compare
|
Tested against ireland with dynamo: just the known testMRJob errors. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
LGTM. I'm =0 on using But I'm not that concerned +1 |
|
Found an unrealted test error: org.apache.hadoop.fs.s3a.s3guard.ITestS3GuardToolLocal#testInitNegativeRead is failing: Given that the failure is unrelated (it fails on trunk as well) I will create a new jira for it and this can go in. |
|
Created https://issues.apache.org/jira/browse/HADOOP-16436 for the unrelated test failure. |
…MetadataStore interface. Contributed by Gabor Bota. (apache#1009) (cherry picked from commit c58e11b) Change-Id: I8e2589c539c635f36e128029e8d5ffdcbdbc2994
…MetadataStore interface. Contributed by Gabor Bota. (apache#1009)
…MetadataStore interface