Skip to content

Conversation

@fapifta
Copy link
Contributor

@fapifta fapifta commented Jul 11, 2020

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.

@avijayanhwx
Copy link
Contributor

avijayanhwx commented Jul 13, 2020

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.

@fapifta
Copy link
Contributor Author

fapifta commented Jul 14, 2020

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.

Copy link
Contributor

@avijayanhwx avijayanhwx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1. I will merge it after a clean CI.

@avijayanhwx
Copy link
Contributor

/retest

@fapifta
Copy link
Contributor Author

fapifta commented Jul 14, 2020

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.

@adoroszlai
Copy link
Contributor

/retest does not work (#1137)

@avijayanhwx avijayanhwx merged commit 0a1cce5 into apache:master Jul 14, 2020
@avijayanhwx
Copy link
Contributor

Thank you for fixing this @fapifta. I have merged your patch.

ChenSammi pushed a commit that referenced this pull request Jul 22, 2020
…her than rely on proto serialization for key. (#1197)

(cherry picked from commit 0a1cce5)
@fapifta fapifta deleted the HDDS-3925 branch July 23, 2020 11:50
rakeshadr pushed a commit to rakeshadr/hadoop-ozone that referenced this pull request Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants