[GitHub] [ozone] errose28 commented on a change in pull request #1933: HDDS-4817. Fresh deploy of Ozone must use the highest layout version by default.

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

errose28 commented on a change in pull request #1933:
URL: https://github.com/apache/ozone/pull/1933#discussion_r583204795



##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/upgrade/TestDataNodeStartupSlvLessThanMlv.java
##########
@@ -48,8 +50,7 @@
   public void testStartupSlvLessThanMlv() throws Exception {
     // Add subdirectories under the temporary folder where the version file
     // will be placed.
-    File datanodeSubdir = tempFolder.newFolder("datanodeStorageConfig",
-        "current");
+    File datanodeSubdir = tempFolder.newFolder(DATANODE_LAYOUT_VERSION_DIR);
 
     OzoneConfiguration conf = new OzoneConfiguration();
     conf.set(HddsConfigKeys.OZONE_METADATA_DIRS,

Review comment:
       I can't comment on the actual code since its unchanged, but it looks like the for loop on line 61 could be replaced with the new `HDDSLayoutVersionManager#maxLayoutVersion` method?

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/HDDSVolumeLayoutVersion.java
##########
@@ -21,11 +21,11 @@
  * Datanode layout version which describes information about the layout version
  * on the datanode.
  */

Review comment:
       Nit: Update doc string since this class now deals with volumes, not the whole datanode.

##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java
##########
@@ -211,15 +211,17 @@ public void testScmLayoutOnRegister()
       throws IOException, InterruptedException, AuthenticationException {
 
     try (SCMNodeManager nodeManager = createNodeManager(getConf())) {
-      Integer nodeManagerSoftwareLayoutVersion =
-          nodeManager.getLayoutVersionManager().getSoftwareLayoutVersion();
-      LayoutVersionProto layoutInfoSuccess = LayoutVersionProto.newBuilder()
-          .setMetadataLayoutVersion(1)
-          .setSoftwareLayoutVersion(nodeManagerSoftwareLayoutVersion).build();
-      LayoutVersionProto layoutInfoFailure = LayoutVersionProto.newBuilder()
-          .setMetadataLayoutVersion(1)
-          .setSoftwareLayoutVersion(nodeManagerSoftwareLayoutVersion + 1)
-          .build();
+      HDDSLayoutVersionManager layoutVersionManager =
+          nodeManager.getLayoutVersionManager();
+      int nodeManagerMetadataLayoutVersion =
+          layoutVersionManager.getMetadataLayoutVersion();
+      int nodeManagerSoftwareLayoutVersion =
+          layoutVersionManager.getSoftwareLayoutVersion();
+      LayoutVersionProto layoutInfoSuccess = toLayoutVersionProto(
+          nodeManagerMetadataLayoutVersion, nodeManagerSoftwareLayoutVersion);
+      LayoutVersionProto layoutInfoFailure = toLayoutVersionProto(
+          nodeManagerSoftwareLayoutVersion + 1,

Review comment:
       Should this be `nodeManagerMetadataLayoutVersion`?

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/upgrade/AbstractLayoutVersionManager.java
##########
@@ -40,33 +42,38 @@
 public abstract class AbstractLayoutVersionManager<T extends LayoutFeature>
     implements LayoutVersionManager, LayoutVersionManagerMXBean {
 
+  private static final Logger LOG =
+      LoggerFactory.getLogger(AbstractLayoutVersionManager.class);
+
   protected int metadataLayoutVersion; // MLV.
   protected int softwareLayoutVersion; // SLV.
   protected TreeMap<Integer, T> features = new TreeMap<>();
   protected Map<String, T> featureMap = new HashMap<>();
-  protected volatile boolean isInitialized = false;
-  protected volatile Status currentUpgradeState =
-      FINALIZATION_REQUIRED;
+  protected volatile Status currentUpgradeState = FINALIZATION_REQUIRED;
 
   protected void init(int version, T[] lfs) throws IOException {
 
-    if (!isInitialized) {
-      metadataLayoutVersion = version;
-      initializeFeatures(lfs);
-      softwareLayoutVersion = features.lastKey();
-      isInitialized = true;
-      if (softwareIsBehindMetaData()) {
-        throw new IOException(
-            String.format("Cannot initialize VersionManager. Metadata " +
-                    "layout version (%d) > software layout version (%d)",
-                metadataLayoutVersion, softwareLayoutVersion));
-      } else if (metadataLayoutVersion == softwareLayoutVersion) {
-        currentUpgradeState = ALREADY_FINALIZED;
-      }
+    metadataLayoutVersion = version;
+    initializeFeatures(lfs);
+    softwareLayoutVersion = features.lastKey();
+    if (softwareIsBehindMetaData()) {
+      throw new IOException(
+          String.format("Cannot initialize VersionManager. Metadata " +
+                  "layout version (%d) > software layout version (%d)",
+              metadataLayoutVersion, softwareLayoutVersion));
+    } else if (metadataLayoutVersion == softwareLayoutVersion) {
+      currentUpgradeState = ALREADY_FINALIZED;
     }
 
+    LayoutFeature mlvFeature = features.get(metadataLayoutVersion);
+    LayoutFeature slvFeature = features.get(softwareLayoutVersion);
+    LOG.info("Initializing Layout version manager with metadata layout" +
+        " = {} (version = {}), software layout = {} (version = {})",
+        mlvFeature, mlvFeature.layoutVersion(),
+        slvFeature, slvFeature.layoutVersion());

Review comment:
       Logging mlvFeature and slvFeature is just going to give an object ID when logged, right? We want the description or something else that gives a meaningful String?

##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java
##########
@@ -696,7 +702,12 @@ void initializeOmStorage(OMStorage omStorage) throws IOException {
     protected OzoneManager createOM()

Review comment:
       Looks like this doesn't get called by MiniOzoneHACluster, which uses createOMService() instead. Probably need a minor modification in that class to call this so we can set version in HA clusters.

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/upgrade/OMLayoutFeatureUtil.java
##########
@@ -39,7 +37,7 @@
    * "disallowed" by just adding the following annotation, thereby keeping the
    * method logic and upgrade logic separate.
    */
-  @DisallowedUntilLayoutVersion(ERASURE_CODING)
+  //@DisallowedUntilLayoutVersion(ERASURE_CODING)

Review comment:
       Looks like this class is being used for demonstration/testing purposes only. Should we move it out of *src* and under *test* with the test that uses it?




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