[GitHub] [hadoop-ozone] avijayanhwx commented on a change in pull request #1457: HDDS-4253. Add LayoutVersion request/response for DN registration.

[GitBox <gi...@apache.org>]

avijayanhwx commented on a change in pull request #1457:
URL: https://github.com/apache/hadoop-ozone/pull/1457#discussion_r497840950



##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1149,4 +1155,8 @@ public String getScmId() {
   public String getClusterId() {
     return getScmStorageConfig().getClusterID();
   }
+
+  public HDDSLayoutVersionManager getLayoutVersionManager() {

Review comment:
       Nit. I don't see any usages for this yet.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
##########
@@ -240,8 +247,19 @@ public VersionResponse getVersion(SCMVersionRequestProto versionRequest) {
   @Override
   public RegisteredCommand register(
       DatanodeDetails datanodeDetails, NodeReportProto nodeReport,
-      PipelineReportsProto pipelineReportsProto) {
-
+      PipelineReportsProto pipelineReportsProto,
+      LayoutVersionProto layoutInfo) {
+
+    if (layoutInfo != null) {

Review comment:
       Can we move this check to org.apache.hadoop.hdds.scm.server.SCMDatanodeProtocolServer#register? That way, a lot of test changes in this patch to incorporate the 4th parameter to NodeManager.register() can be avoided.

##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java
##########
@@ -170,6 +176,33 @@ public void testScmHeartbeat()
     }
   }
 
+  /**
+   * Tests that Node manager handles Layout versions correctly.
+   *
+   * @throws IOException
+   * @throws InterruptedException
+   * @throws TimeoutException
+   */
+  @Test
+  public void testScmLayoutOnRegister()
+      throws IOException, InterruptedException, AuthenticationException {
+
+    try (SCMNodeManager nodeManager = createNodeManager(getConf())) {
+      LayoutVersionProto layoutInfoSuccess = LayoutVersionProto.newBuilder()
+          .setMetadataLayoutVersion(1).setSoftwareLayoutVersion(1).build();

Review comment:
       Can we refactor this test to use last layout feature's version and last layout feature's version + 1 for success and failure cases? That way, the test does not need change every time a Layout feature is added.




----------------------------------------------------------------
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