-
Notifications
You must be signed in to change notification settings - Fork 519
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
HDDS-4478. Large deletedKeyset slows down OM via listStatus. #1598
Conversation
I am also planning to install this patch on the cluster I ran into the problem, and check if the problem is fixed there as well. |
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.
Good catch @fapifta
Over all LGTM. Few comments inline.
/** | ||
* Response for crate file request. | ||
*/ | ||
@CleanupTableInfo(cleanupTables = {KEY_TABLE, OPEN_KEY_TABLE}) |
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.
KEY_TABLE is needed for KeyCreate also, as when ozone.om.enable.filesystem.paths is true, directories are created for KeyCreate also.
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.
Hmm... yes, you are right, key create I have missed for this one...
Let me add that along with a test tomorrow.
OMFileCreateResponse.class.getAnnotation(CleanupTableInfo.class); | ||
List<String> cleanup = Arrays.asList(ann.cleanupTables()); | ||
for (String tableName : omMetaMgr.listTableNames()) { | ||
if (!cleanup.contains(tableName)) { |
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.
Here we have checked only tables which are not part of FileCreateResponse cleanupTable annotation.
Do we want to check tables which are affected also.
Just a question, not got what these lines are testing? (Is it just to see any tables which are not affected have same size in Cache) But how this is verifying fix, not sure if i am missing something basic here.
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.
Yes, the test does what you have summarized.
The basic idea is the following:
The issue is that we have certain epochs that are pushing entries to unexpected table caches. Unexpected in a way that eviction for those epochs on particular tables is not called when the DoubleBuffer flushes, because of the missing table name in the annotation.
I think it is sufficient to check whether we have added any unexpected cache entries to any other table's cache during applyTransaction. I might be wrong on this one, or there might be an easier way though.
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.
Ya, it makes sense to me. It is basically checking all other table cache entries should be what it has an initial value, which should not be changed.
Thank you for the review @bharatviswa504 please find my answers inline, and expect the addition of key create case tomorrow. |
Refactored the test to make the real test methods easy to understand and free from setup clutter as much as possible.
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.
+1 LGTM
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.
Thank You @fapifta for the contribution.
* HDDS-3698-upgrade: (46 commits) HDDS-4468. Fix Goofys listBucket large than 1000 objects will stuck forever (apache#1595) HDDS-4417. Simplify Ozone client code with configuration object -- addendum (apache#1581) HDDS-4476. Improve the ZH translation of the HA.md in doc. (apache#1597) HDDS-4432. Update Ratis version to latest snapshot. (apache#1586) HDDS-4488. Open RocksDB read only when loading containers at Datanode startup (apache#1605) HDDS-4478. Large deletedKeyset slows down OM via listStatus. (apache#1598) HDDS-4452. findbugs.sh couldn't be executed after a full build (apache#1576) HDDS-4427. Avoid ContainerCache in ContainerReader at Datanode startup (apache#1549) HDDS-4448. Duplicate refreshPipeline in listStatus (apache#1569) HDDS-4450. Cannot run ozone if HADOOP_HOME points to Hadoop install (apache#1572) HDDS-4346.Ozone specific Trash Policy (apache#1535) HDDS-4426. SCM should create transactions using all blocks received from OM (apache#1561) HDDS-4399. Safe mode rule for piplelines should only consider open pipelines. (apache#1526) HDDS-4367. Configuration for deletion service intervals should be different for OM, SCM and datanodes (apache#1573) HDDS-4462. Add --frozen-lockfile to pnpm install to prevent ozone-recon-web/pnpm-lock.yaml from being updated automatically (apache#1589) HDDS-4082. Create ZH translation of HA.md in doc. (apache#1591) HDDS-4464. Upgrade httpclient version due to CVE-2020-13956. (apache#1590) HDDS-4467. Acceptance test fails due to new Hadoop 3 image (apache#1594) HDDS-4466. Update url in .asf.yaml to use TLP project (apache#1592) HDDS-4458. Fix Max Transaction ID value in OM. (apache#1585) ...
* HDDS-3698-upgrade: (47 commits) HDDS-4468. Fix Goofys listBucket large than 1000 objects will stuck forever (apache#1595) HDDS-4417. Simplify Ozone client code with configuration object -- addendum (apache#1581) HDDS-4476. Improve the ZH translation of the HA.md in doc. (apache#1597) HDDS-4432. Update Ratis version to latest snapshot. (apache#1586) HDDS-4488. Open RocksDB read only when loading containers at Datanode startup (apache#1605) HDDS-4478. Large deletedKeyset slows down OM via listStatus. (apache#1598) HDDS-4452. findbugs.sh couldn't be executed after a full build (apache#1576) HDDS-4427. Avoid ContainerCache in ContainerReader at Datanode startup (apache#1549) HDDS-4448. Duplicate refreshPipeline in listStatus (apache#1569) HDDS-4450. Cannot run ozone if HADOOP_HOME points to Hadoop install (apache#1572) HDDS-4346.Ozone specific Trash Policy (apache#1535) HDDS-4426. SCM should create transactions using all blocks received from OM (apache#1561) HDDS-4399. Safe mode rule for piplelines should only consider open pipelines. (apache#1526) HDDS-4367. Configuration for deletion service intervals should be different for OM, SCM and datanodes (apache#1573) HDDS-4462. Add --frozen-lockfile to pnpm install to prevent ozone-recon-web/pnpm-lock.yaml from being updated automatically (apache#1589) HDDS-4082. Create ZH translation of HA.md in doc. (apache#1591) HDDS-4464. Upgrade httpclient version due to CVE-2020-13956. (apache#1590) HDDS-4467. Acceptance test fails due to new Hadoop 3 image (apache#1594) HDDS-4466. Update url in .asf.yaml to use TLP project (apache#1592) HDDS-4458. Fix Max Transaction ID value in OM. (apache#1585) ...
What changes were proposed in this pull request?
See details about the issue found in the JIRA, the change proposed is fairly simple, as the key table is missing from the CleanupTableInfo annotation values, which prevents cache eviction in the OMDoubleBuffer logic.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4478
How was this patch tested?
The JUnit test I wrote tests this particular scenario with the given request. I am posting a follow up JIRA for a most comprehensive test that covers probably all the request - response pairs somehow. Or at least as an outcome, helps us detect these kind of problems easier and earlier.