HADOOP-16520 dynamodb ms version race refactor. #1576
Merged
Conversation
This file contains 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
|
yeah, anything which avoids creating duplicate tables is good. I did some of that the last time I worked on ITestDynamoDBMetadataStore |
...op-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStoreTableHandler.java
Outdated
Show resolved
Hide resolved
...op-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStoreTableHandler.java
Outdated
Show resolved
Hide resolved
...op-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStoreTableHandler.java
Outdated
Show resolved
Hide resolved
|
|
||
| if (versionMarkerItem == null && versionMarkerFromTag == null) { | ||
| if (!isEmptyTable(tableName, amazonDynamoDB)) { | ||
| LOG.error("Table is not empty but missing the version maker. Failing."); |
There was a problem hiding this comment.
we need to make sure the s3guard docs are up to date with these new error messages
...op-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStoreTableHandler.java
Outdated
Show resolved
Hide resolved
...op-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStoreTableHandler.java
Outdated
Show resolved
Hide resolved
...op-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStoreTableHandler.java
Outdated
Show resolved
Hide resolved
...op-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStoreTableHandler.java
Outdated
Show resolved
Hide resolved
|
I tested out on the command line while doing things to the DDB tables " Version marker in the dynamo table was null" How about "No version marker found in the DynamoDB table " + table name? Issue: we always patch the table, even on fsck, destroy and bucket-info calls. Should we? I'm happy with it as is, as trying to be clever complicates life. Issue: What do we think makes a good retry interval? I think it is too big, as a bucket info is now sleeping for 3 minutes before giving up. (that's on a table with no version Tag BTW) . I verified that incompatible item or tag values raised errors
In those situations, I think the error could include the full details which are logged at debug. I can see that being useful. CLI run on an inconsistent version marker (even if there's a valid file entry) is noisy: it logs at error with stack and then throws. I'd prefer just the log without the stack here, Side issue a prune with seconds 0 didn't delete tombstones from the DDB table. But when I said -seconds 1, it did I don't think it is at all related -but have a try on your system and see what happens. |
|
I should add -overall patch LGTM; these are very minor changes. Impressed that the tag update means that you get this version resilience automatically, one thought there: should we think about tagging the table explicitly with some "s3guard" string, e.g "s3guard-version" - makes it clearer to admins looking at tables which are guarded |
|
steveloughran
changed the title
|
One change for this: can we stop telling the user all is good at info? It's the good state. I saw this on a bucket-info command where this is the first message everyone sees. Assuming this is constant for all other commands, it's a distraction |
added 5 commits
October 9, 2019 14:16
…compile and verify OK after refactor Change-Id: Ia83e92b9039ccb780090c99c41b4f71ef7539d35
Change-Id: I34704f7def441af72a1b85be44fcb91fc8d2b2ce
Change-Id: I3ab87ca8c31d9e06ec1562691edd316536cfc883
Change-Id: I924347a550cecf991d37c9a1a8f1f1071967c3db
…meout for version marker ITEM check Change-Id: Idf960a0c0541c2d2088cbcafe045f6d43f366383
Change-Id: Ief4db248f46be18d1838743dc0b212d6a4bbfd83
bgaborg
force-pushed
the
HADOOP-16520-dynamodb-ms-version-race-refactor
branch
from
fffc5e9
to
ecd49f4
Compare
|
last patch:
|
|
💔 -1 overall
This message was automatically generated. |
Change-Id: Ifce37b942f3a929dbf7e7ab9278f1ba0be22f95b
|
tested against ireland with no errors. |
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran can you take a look at this? |
|
last commit:
|
| @@ -974,6 +1001,8 @@ in an incompatible manner. The version marker in tables exists to support | |||
| such an option if it ever becomes necessary, by ensuring that all S3Guard | |||
| client can recognise any version mismatch. | |||
|
|
|||
| * Table versionin | |||
There was a problem hiding this comment.
looks like a leftover line
steveloughran
approved these changes
Oct 11, 2019
Change-Id: I94046bd08b0d22969a61ed8b6f0c7ec087ab0ac9
|
Thanks. I removed the leftover line from the docs, and merge this change. |
amahussein
pushed a commit
to amahussein/hadoop
that referenced
this pull request
Oct 29, 2019
…pache#1576). Contributed by Gabor Bota. Fixes HADOOP-16349. DynamoDBMetadataStore.getVersionMarkerItem() to log at info/warn on retry Change-Id: Ia83e92b9039ccb780090c99c41b4f71ef7539d35
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
Feb 2, 2020
…pache#1576). Contributed by Gabor Bota. Fixes HADOOP-16349. DynamoDBMetadataStore.getVersionMarkerItem() to log at info/warn on retry Change-Id: Ia83e92b9039ccb780090c99c41b4f71ef7539d35 (cherry picked from commit 4a700c2)
RogPodge
pushed a commit
to RogPodge/hadoop
that referenced
this pull request
Mar 25, 2020
…pache#1576). Contributed by Gabor Bota. Fixes HADOOP-16349. DynamoDBMetadataStore.getVersionMarkerItem() to log at info/warn on retry Change-Id: Ia83e92b9039ccb780090c99c41b4f71ef7539d35
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 following should be included: HADOOP-16349. DynamoDBMetadataStore.getVersionMarkerItem() to log at info/warn on retry.
Notes:
Why there's no ITestDynamoDBMetadataStoreTableHandler if there's ITestDynamoDBMetadataStore, and why do I added the tests there?
For readability and to avoid duplication. DynamoDBMetadataStoreTableHandler is just basically parts factored out from DynamoDBMetadataStore, and some helper methods.
Why there's only testTableVersioning->checkVerifyVersionMarkerCompatibility to cover the full functionality of the version marker checking and recovery with dynamo, and not 6 different methods?
Creating and deleting tables takes a lot of time. Minutes. I use the same table during the test and add/remove the version tag and item to cover all combinations.
Why there's log on info level at DynamoDBMetadataStoreTableHandler#verifyVersionCompatibility?
I think it's useful for know what's going on with the versions. It's not a lot logging at that level, but I find it useful.
Things to add:
Tested against ireland. Some tables won't be deleted for me so I get a timeout. I don't think that's related, maybe I'm just over the limit of creating and deleting tables now.