HDDS-4296. SCM changes to process Layout Info in heartbeat request/response #1486
Conversation
linyiqun
left a comment
linyiqun
left a comment
There was a problem hiding this comment.
Hi @prashantpogde , some initial review comments from me.
It would be better if you can additionally add corresponding unit test for this PR change.
...-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/AbstractLayoutVersionManager.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/OzoneUpgradeState.java
Outdated
Show resolved
Hide resolved
...dds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/LayoutVersionReportHandler.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
Outdated
Show resolved
Hide resolved
avijayanhwx
left a comment
avijayanhwx
left a comment
There was a problem hiding this comment.
Thanks for working on this @prashantpogde. A couple of general comments.
-
@fapifta has also introduced states a component can be in, with respect to Upgrade. We should discuss and use just one set of states.
-
I am not convinced why we need another report for Layout Version handling. Layout version is like node metadata, and should be sent in every heartbeat. Also, I am not sure if we can handle the LayoutVersion report through an event handler since it is 'async' and there can be delay in handling.
prashantpogde
force-pushed
the
HDDS-4296
branch
from
6030f09 to
8053652
Compare
|
Updated with new set of changes after taking care of all review comments. Please take a look. |
avijayanhwx
left a comment
avijayanhwx
left a comment
There was a problem hiding this comment.
@prashantpogde Patch generally looks good to me. Can we add a unit test for the layout version check during heartbeat processing?
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
Show resolved
Hide resolved
avijayanhwx
left a comment
avijayanhwx
left a comment
There was a problem hiding this comment.
LGTM +1.
@prashantpogde Are you planning to comment out the other 2 unused variable findbug failures as well?
Can we trigger an empty commit to get acceptance tests(unsecure) to pass?
|
Failure in TestOzoneManagerHAMetadataOnly is unrelated with the changes. |
|
Thank you for the patch @prashantpogde and the review @linyiqun. |
3 participants
What changes were proposed in this pull request?
SCM changes to process Layout Info in heartbeat request/response
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4296
How was this patch tested?
UT. I will fix any CI failure.