You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/11/17 16:40:30 UTC
[GitHub] [ozone] fapifta opened a new pull request #1598: HDDS-4478. Large deletedKeyset slows down OM via listStatus.
fapifta opened a new pull request #1598:
URL: https://github.com/apache/ozone/pull/1598
## 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a change in pull request #1598: HDDS-4478. Large deletedKeyset slows down OM via listStatus.
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #1598:
URL: https://github.com/apache/ozone/pull/1598#discussion_r525612877
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java
##########
@@ -23,15 +23,20 @@
import org.apache.hadoop.ozone.om.helpers.OmBucketInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
+import org.apache.hadoop.ozone.om.response.CleanupTableInfo;
import org.apache.hadoop.ozone.om.response.key.OMKeyCreateResponse;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos
.OMResponse;
import java.util.List;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.KEY_TABLE;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.OPEN_KEY_TABLE;
+
/**
* Response for crate file request.
*/
+@CleanupTableInfo(cleanupTables = {KEY_TABLE, OPEN_KEY_TABLE})
Review comment:
Hmm... yes, you are right, key create I have missed for this one...
Let me add that along with a test tomorrow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a change in pull request #1598: HDDS-4478. Large deletedKeyset slows down OM via listStatus.
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #1598:
URL: https://github.com/apache/ozone/pull/1598#discussion_r525614720
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java
##########
@@ -66,4 +105,188 @@ public void checkAnnotationAndTableName() throws Exception {
}
});
}
+
+ @Test
+ public void testHDDS4478() throws Exception {
+ HddsProtos.BlockID blockID = new BlockID(1, 1).getProtobuf();
+ String volume = "testVol";
+ String bucket = "testBuck";
+ String key = "/foo/bar/baz/key";
+
+
+ OMFileCreateRequest request =
+ anOmFileCreateRequest(blockID, volume, bucket, key);
+
+ OMMetadataManager omMetaMgr = createOMMetadataManagerSpy();
+ OMMetrics omMetrics = mock(OMMetrics.class);
+ OzoneManager om =
+ createOzoneManagerMock(volume, bucket, request, omMetaMgr, omMetrics);
+
+ OmVolumeArgs volumeArgs = aVolumeArgs(volume);
+ OmBucketInfo bucketInfo = aBucketInfo(volume, bucket);
+ addVolumeToMetaTable(volume, volumeArgs, omMetaMgr);
+ addBucketToMetaTable(volume, bucket, bucketInfo, omMetaMgr);
+
+ OzoneManagerDoubleBufferHelper dbh =
+ mock(OzoneManagerDoubleBufferHelper.class);
+
+ Map<String, Integer> cacheItemCount = new HashMap<>();
+ for (String tableName : omMetaMgr.listTableNames()){
+ cacheItemCount.put(tableName,
+ Iterators.size(omMetaMgr.getTable(tableName).cacheIterator()));
+ }
+
+
+ request.validateAndUpdateCache(om, 1, dbh);
+
+
+ CleanupTableInfo ann =
+ OMFileCreateResponse.class.getAnnotation(CleanupTableInfo.class);
+ List<String> cleanup = Arrays.asList(ann.cleanupTables());
+ for (String tableName : omMetaMgr.listTableNames()) {
+ if (!cleanup.contains(tableName)) {
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on pull request #1598: HDDS-4478. Large deletedKeyset slows down OM via listStatus.
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #1598:
URL: https://github.com/apache/ozone/pull/1598#issuecomment-729058519
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on pull request #1598: HDDS-4478. Large deletedKeyset slows down OM via listStatus.
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #1598:
URL: https://github.com/apache/ozone/pull/1598#issuecomment-729297722
Thank you for the review @bharatviswa504 please find my answers inline, and expect the addition of key create case tomorrow.
While working on that one, I will try to come up with a more general solution, if the overall idea of how to test this seems suitable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1598: HDDS-4478. Large deletedKeyset slows down OM via listStatus.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1598:
URL: https://github.com/apache/ozone/pull/1598#discussion_r525595043
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java
##########
@@ -23,15 +23,20 @@
import org.apache.hadoop.ozone.om.helpers.OmBucketInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
+import org.apache.hadoop.ozone.om.response.CleanupTableInfo;
import org.apache.hadoop.ozone.om.response.key.OMKeyCreateResponse;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos
.OMResponse;
import java.util.List;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.KEY_TABLE;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.OPEN_KEY_TABLE;
+
/**
* Response for crate file request.
*/
+@CleanupTableInfo(cleanupTables = {KEY_TABLE, OPEN_KEY_TABLE})
Review comment:
KEY_TABLE is needed for KeyCreate also, as when ozone.om.enable.filesystem.paths is true, directories are created for KeyCreate also.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 merged pull request #1598: HDDS-4478. Large deletedKeyset slows down OM via listStatus.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #1598:
URL: https://github.com/apache/ozone/pull/1598
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1598: HDDS-4478. Large deletedKeyset slows down OM via listStatus.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1598:
URL: https://github.com/apache/ozone/pull/1598#discussion_r525763319
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java
##########
@@ -66,4 +105,188 @@ public void checkAnnotationAndTableName() throws Exception {
}
});
}
+
+ @Test
+ public void testHDDS4478() throws Exception {
+ HddsProtos.BlockID blockID = new BlockID(1, 1).getProtobuf();
+ String volume = "testVol";
+ String bucket = "testBuck";
+ String key = "/foo/bar/baz/key";
+
+
+ OMFileCreateRequest request =
+ anOmFileCreateRequest(blockID, volume, bucket, key);
+
+ OMMetadataManager omMetaMgr = createOMMetadataManagerSpy();
+ OMMetrics omMetrics = mock(OMMetrics.class);
+ OzoneManager om =
+ createOzoneManagerMock(volume, bucket, request, omMetaMgr, omMetrics);
+
+ OmVolumeArgs volumeArgs = aVolumeArgs(volume);
+ OmBucketInfo bucketInfo = aBucketInfo(volume, bucket);
+ addVolumeToMetaTable(volume, volumeArgs, omMetaMgr);
+ addBucketToMetaTable(volume, bucket, bucketInfo, omMetaMgr);
+
+ OzoneManagerDoubleBufferHelper dbh =
+ mock(OzoneManagerDoubleBufferHelper.class);
+
+ Map<String, Integer> cacheItemCount = new HashMap<>();
+ for (String tableName : omMetaMgr.listTableNames()){
+ cacheItemCount.put(tableName,
+ Iterators.size(omMetaMgr.getTable(tableName).cacheIterator()));
+ }
+
+
+ request.validateAndUpdateCache(om, 1, dbh);
+
+
+ CleanupTableInfo ann =
+ OMFileCreateResponse.class.getAnnotation(CleanupTableInfo.class);
+ List<String> cleanup = Arrays.asList(ann.cleanupTables());
+ for (String tableName : omMetaMgr.listTableNames()) {
+ if (!cleanup.contains(tableName)) {
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1598: HDDS-4478. Large deletedKeyset slows down OM via listStatus.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1598:
URL: https://github.com/apache/ozone/pull/1598#discussion_r525595043
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java
##########
@@ -23,15 +23,20 @@
import org.apache.hadoop.ozone.om.helpers.OmBucketInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
+import org.apache.hadoop.ozone.om.response.CleanupTableInfo;
import org.apache.hadoop.ozone.om.response.key.OMKeyCreateResponse;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos
.OMResponse;
import java.util.List;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.KEY_TABLE;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.OPEN_KEY_TABLE;
+
/**
* Response for crate file request.
*/
+@CleanupTableInfo(cleanupTables = {KEY_TABLE, OPEN_KEY_TABLE})
Review comment:
KEY_TABLE is needed for KeyCreate also, as when ozone.om.enable.filesystem.paths is true.
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java
##########
@@ -66,4 +105,188 @@ public void checkAnnotationAndTableName() throws Exception {
}
});
}
+
+ @Test
+ public void testHDDS4478() throws Exception {
+ HddsProtos.BlockID blockID = new BlockID(1, 1).getProtobuf();
+ String volume = "testVol";
+ String bucket = "testBuck";
+ String key = "/foo/bar/baz/key";
+
+
+ OMFileCreateRequest request =
+ anOmFileCreateRequest(blockID, volume, bucket, key);
+
+ OMMetadataManager omMetaMgr = createOMMetadataManagerSpy();
+ OMMetrics omMetrics = mock(OMMetrics.class);
+ OzoneManager om =
+ createOzoneManagerMock(volume, bucket, request, omMetaMgr, omMetrics);
+
+ OmVolumeArgs volumeArgs = aVolumeArgs(volume);
+ OmBucketInfo bucketInfo = aBucketInfo(volume, bucket);
+ addVolumeToMetaTable(volume, volumeArgs, omMetaMgr);
+ addBucketToMetaTable(volume, bucket, bucketInfo, omMetaMgr);
+
+ OzoneManagerDoubleBufferHelper dbh =
+ mock(OzoneManagerDoubleBufferHelper.class);
+
+ Map<String, Integer> cacheItemCount = new HashMap<>();
+ for (String tableName : omMetaMgr.listTableNames()){
+ cacheItemCount.put(tableName,
+ Iterators.size(omMetaMgr.getTable(tableName).cacheIterator()));
+ }
+
+
+ request.validateAndUpdateCache(om, 1, dbh);
+
+
+ CleanupTableInfo ann =
+ OMFileCreateResponse.class.getAnnotation(CleanupTableInfo.class);
+ List<String> cleanup = Arrays.asList(ann.cleanupTables());
+ for (String tableName : omMetaMgr.listTableNames()) {
+ if (!cleanup.contains(tableName)) {
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org