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/07/11 01:47:04 UTC
[GitHub] [hadoop-ozone] fapifta opened a new pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
fapifta opened a new pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197
## What changes were proposed in this pull request?
As we recently learned, protoBuf serialization is not needed to produce the same byte array over the wire, and therefore the toByteArray method's return value should not be used to byte array based comparison of protobuf serializations.
In the SCM's RocksDB we use the PipelineID as a key in the Pipeline table, and the key is created using the byte array representation of the protobuf serialization of the PipelineID, which can lead to mismatches when we access anything from the table based on a key. At the moment this can prevent us to delete a Pipeline from the table if there is a change in the byte array representation.
In order to avoid this situation, the way how we create the key to this table from the PipelineID needs to be changed, this is one part of this PR, and covered in the PipelineIDCodec related changes.
The other part is to clean up the DB from the old keys, as SCM will not be able to close and remove those Pipeline based on the ID as after we change the key serialization the byte array matched and stored will not be the same for the same PipelineID, that is used in the delete method which the code uses. So SCM can end up with pipelines that will never be cleaned up from the DB, and are polluting the in memory structures at startup until they are considered invalid.
In order to avoid this, SCMPipelineManager from now on checks if the key deserialization results in the same PipelineID as the one stored in the value in the table which is a Pipeline object. If the two are different, we remove the old version from the table, and add it with the new key, so that the pipelines are still preserved, but later can be deleted from RocksDB as well by the SCM when appropriate.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3925
## How was this patch tested?
JUnit tests are added to new things. And to check key migration happens properly.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#issuecomment-658315973
`/retest` does not work (#1137)
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx merged pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
avijayanhwx merged pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx commented on pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#issuecomment-658298859
/retest
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx edited a comment on pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
avijayanhwx edited a comment on pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#issuecomment-657780236
Thank you working on this pifta. I have verified the working using docker based testing.
LGTM +1.
Can we add a unit test for to verify that removeFromDb actually removes the entry? I am OK with adding it through a follow up JIRA.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] fapifta commented on pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#issuecomment-658315099
Thank you for the review @avijayanhwx, I haven't seen the CI checks running, so I pushed the branch once more, let's see, all failures seems irrelevant, especially after a clean build was there already before I have added the new test.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] fapifta commented on pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#issuecomment-657903257
Hi @avijayanhwx,
thank you for the review, I have pushed the requested test, and a bit more.
At the end of the day, I have added tests to verify the behaviour and interactions of RDBStoreIterator with the underlying RockIterator, and the RocksDBTable. I hope this should sufficiently address the test request, let me know if you thought about something different.
As the TypedTable.TypedTableIterator class purely delegates to the raw RDbStoreIterator, I think that does not require too much tests.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx edited a comment on pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
avijayanhwx edited a comment on pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#issuecomment-657780236
Thank you working on this pifta. The patch looks good to me. I have verified the working using docker based testing.
Can we add a unit test for to verify that removeFromDb actually removes the entry? I am OK with adding it through a follow up JIRA.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] fapifta removed a comment on pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
fapifta removed a comment on pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#issuecomment-658059361
/retest
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx commented on pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#issuecomment-657780236
Thank you working on this pifta. The patch looks good to me. I have verified the working using docker based testing. Can we add a unit test for to verify that removeFromDb actually removes the entry?
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx commented on a change in pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#discussion_r454505312
##########
File path: hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestRDBStoreIterator.java
##########
@@ -0,0 +1,224 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
Review comment:
Thanks for adding this test @fapifta.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] fapifta commented on pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#issuecomment-658059361
/retest
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] avijayanhwx commented on pull request #1197: HDDS-3925. SCM Pipeline DB should directly use UUID bytes for key rather than rely on proto serialization for key.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on pull request #1197:
URL: https://github.com/apache/hadoop-ozone/pull/1197#issuecomment-658424073
Thank you for fixing this @fapifta. I have merged your patch.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org