AMBARI-18070. Calculation of versionAdvertised in is incorrect in metainfo.xml when parent is false and current ComponentInfo is true (alejandro)

ambari-server/src/main/java/org/apache/ambari/server/stack/ComponentModule.java

@@ -88,7 +88,14 @@ public class ComponentModule extends BaseModule<ComponentModule, ComponentInfo>
       if (componentInfo.getCardinality() == null) {
         componentInfo.setCardinality(parentInfo.getCardinality());
       }
-      componentInfo.setVersionAdvertised(parentInfo.isVersionAdvertised());
+
+      // Inherit versionAdvertised from the parent if the current Component Info has it as null or "inherit".
+      if (null == componentInfo.getVersionAdvertisedField()) {
+        componentInfo.setVersionAdvertised(parentInfo.isVersionAdvertised());
+      } else {
+        // Set to explicit boolean
+        componentInfo.setVersionAdvertised(componentInfo.getVersionAdvertisedField().booleanValue());
+      }
 
       if (componentInfo.getDecommissionAllowed() == null) {
         componentInfo.setDecommissionAllowed(parentInfo.getDecommissionAllowed());

ambari-server/src/main/java/org/apache/ambari/server/state/ComponentInfo.java

@@ -37,7 +37,10 @@ public class ComponentInfo {
   private String category;
   private boolean deleted;
   private String cardinality = "0+";
-
+  
+  @XmlElement(name="versionAdvertised")
+  private Boolean versionAdvertisedField;
+  
   /**
    * Technically, no component is required to advertise a version. In practice, 
    * Components should advertise a version through a mechanism like hdp-select.
@@ -46,9 +49,11 @@ public class ComponentInfo {
    * Whereas clients will advertise the version when INSTALLED.
    * Some components do not need to advertise a version because it is either redundant, or they don't have a mechanism
    * at the moment. For instance, ZKFC has the same version as NameNode, while AMBARI_METRICS and KERBEROS do not have a mechanism.
+   *
+   * This is the translation of the xml element ["true", "false", null] (note that if a value is not specified,
+   * it will inherit from the parent) into a boolean after actually resolving it.
    */
-  @XmlElements(@XmlElement(name = "versionAdvertised"))
-  private boolean versionAdvertised = false;
+  private boolean versionAdvertisedInternal = false;
 
   /**
    * Used to determine if decommission is allowed
@@ -139,7 +144,8 @@ public class ComponentInfo {
     category = prototype.category;
     deleted = prototype.deleted;
     cardinality = prototype.cardinality;
-    versionAdvertised = prototype.versionAdvertised;
+    versionAdvertisedField = prototype.versionAdvertisedField;
+    versionAdvertisedInternal = prototype.versionAdvertisedInternal;
     decommissionAllowed = prototype.decommissionAllowed;
     clientsToUpdateConfigs = prototype.clientsToUpdateConfigs;
     commandScript = prototype.commandScript;
@@ -309,12 +315,45 @@ public class ComponentInfo {
     return cardinality;
   }
 
+  /**
+   * WARNING: only call this method from unit tests to set the Boolean that would have been read from the xml file.
+   * If you call this function, you must still call {@see org.apache.ambari.server.stack.ComponentModule#resolve()}.
+   * @param versionAdvertisedField
+   */
+  public void setVersionAdvertisedField(Boolean versionAdvertisedField) {
+    this.versionAdvertisedField = versionAdvertisedField;
+  }
+
+  /**
+   * WARNING: only call this from ComponentModule to resolve the boolean (true|false).
+   * In all other classes, use {@seealso isVersionAdvertised}
+   * @return The Boolean for versionAdvertised from the xml file in order to resolve it into a boolean.
+   */
+  public Boolean getVersionAdvertisedField() {
+    return this.versionAdvertisedField;
+  }
+
+  /**
+   * WARNING: only call this from ComponentModule to resolve the boolean (true|false).
+   * @param versionAdvertised Final resolution of whether version is advertised or not.
+   */
   public void setVersionAdvertised(boolean versionAdvertised) {
-    this.versionAdvertised = versionAdvertised;
+    this.versionAdvertisedInternal = versionAdvertised;
   }
 
+  /**
+   * Determine if this component advertises a version. This Boolean has already resolved to true|false depending
+   * on explicitly overriding the value or inheriting from an ancestor.
+   * @return boolean of whether this component advertises a version.
+   */
   public boolean isVersionAdvertised() {
-    return versionAdvertised;
+    if (null != versionAdvertisedField) {
+      return versionAdvertisedField.booleanValue();
+    }
+    // If set to null and has a parent, then the value would have already been resolved and set.
+    // Otherwise, return the default value (false).
+    return this.versionAdvertisedInternal;
+
   }
 
   public String getDecommissionAllowed() {
@@ -367,7 +406,8 @@ public class ComponentInfo {
     if (deleted != that.deleted) return false;
     if (autoDeploy != null ? !autoDeploy.equals(that.autoDeploy) : that.autoDeploy != null) return false;
     if (cardinality != null ? !cardinality.equals(that.cardinality) : that.cardinality != null) return false;
-    if (versionAdvertised != that.versionAdvertised) return false;
+    if (versionAdvertisedField != null ? !versionAdvertisedField.equals(that.versionAdvertisedField) : that.versionAdvertisedField != null) return false;
+    if (versionAdvertisedInternal != that.versionAdvertisedInternal) return false;
     if (decommissionAllowed != null ? !decommissionAllowed.equals(that.decommissionAllowed) : that.decommissionAllowed != null) return false;
     if (reassignAllowed != null ? !reassignAllowed.equals(that.reassignAllowed) : that.reassignAllowed != null) return false;
     if (category != null ? !category.equals(that.category) : that.category != null) return false;
@@ -397,7 +437,6 @@ public class ComponentInfo {
     result = 31 * result + (category != null ? category.hashCode() : 0);
     result = 31 * result + (deleted ? 1 : 0);
     result = 31 * result + (cardinality != null ? cardinality.hashCode() : 0);
-    result = 31 * result + (versionAdvertised ? 1 : 0);
     result = 31 * result + (decommissionAllowed != null ? decommissionAllowed.hashCode() : 0);
     result = 31 * result + (reassignAllowed != null ? reassignAllowed.hashCode() : 0);
     result = 31 * result + (commandScript != null ? commandScript.hashCode() : 0);
@@ -409,6 +448,8 @@ public class ComponentInfo {
     result = 31 * result + (autoDeploy != null ? autoDeploy.hashCode() : 0);
     result = 31 * result + (configDependencies != null ? configDependencies.hashCode() : 0);
     result = 31 * result + (clientConfigFiles != null ? clientConfigFiles.hashCode() : 0);
+    // NULL = 0, TRUE = 2, FALSE = 1
+    result = 31 * result + (versionAdvertisedField != null ? (versionAdvertisedField.booleanValue() ? 2 : 1) : 0);
     return result;
   }
 

ambari-server/src/main/resources/common-services/ATLAS/0.7.0.2.5/metainfo.xml

@@ -27,16 +27,23 @@
         <component>
           <name>ATLAS_SERVER</name>
           <cardinality>1+</cardinality>
+          <versionAdvertised>true</versionAdvertised>
+          <dependencies>
+            <dependency>
+              <name>AMBARI_INFRA/INFRA_SOLR_CLIENT</name>
+              <scope>host</scope>
+              <auto-deploy>
+                <enabled>true</enabled>
+              </auto-deploy>
+            </dependency>
+          </dependencies>
+        </component>
+
+        <component>
+          <name>ATLAS_CLIENT</name>
+          <cardinality>1+</cardinality>
+          <versionAdvertised>true</versionAdvertised>
         </component>
-        <dependencies>
-          <dependency>
-            <name>AMBARI_INFRA/INFRA_SOLR_CLIENT</name>
-            <scope>host</scope>
-            <auto-deploy>
-              <enabled>true</enabled>
-            </auto-deploy>
-          </dependency>
-        </dependencies>
       </components>
 
       <quickLinksConfigurations>

ambari-server/src/test/java/org/apache/ambari/server/stack/ComponentModuleTest.java

@@ -478,6 +478,68 @@ public class ComponentModuleTest {
     assertSame("true", resolveComponent(info, parentInfo).getModuleInfo().getReassignAllowed());
   }
 
+  /**
+   * Test that versionAdvertised is resolved correctly.
+   */
+  @Test
+  public void testResolve_VersionAdvertised() {
+    List<ComponentInfo> components = createComponentInfo(2);
+    ComponentInfo info = components.get(0);
+    ComponentInfo parentInfo = components.get(1);
+
+    // Test cases where the current Component Info explicitly sets the value.
+
+    // 1. Chain of versionAdvertised is: true (parent) -> true (current) => true
+    parentInfo.setVersionAdvertisedField(new Boolean(true));
+    parentInfo.setVersionAdvertised(true);
+    info.setVersionAdvertisedField(new Boolean(true));
+    assertEquals(true, resolveComponent(info, parentInfo).getModuleInfo().isVersionAdvertised());
+
+    // 2. Chain of versionAdvertised is: true (parent) -> false (current) => false
+    parentInfo.setVersionAdvertisedField(new Boolean(true));
+    parentInfo.setVersionAdvertised(true);
+    info.setVersionAdvertisedField(new Boolean(false));
+    assertEquals(false, resolveComponent(info, parentInfo).getModuleInfo().isVersionAdvertised());
+
+    // 3. Chain of versionAdvertised is: false (parent) -> true (current) => true
+    parentInfo.setVersionAdvertisedField(new Boolean(false));
+    parentInfo.setVersionAdvertised(false);
+    info.setVersionAdvertisedField(new Boolean(true));
+    assertEquals(true, resolveComponent(info, parentInfo).getModuleInfo().isVersionAdvertised());
+
+    // 4. Chain of versionAdvertised is: null (parent) -> true (current) => true
+    parentInfo.setVersionAdvertisedField(null);
+    parentInfo.setVersionAdvertised(false);
+    info.setVersionAdvertisedField(new Boolean(true));
+    assertEquals(true, resolveComponent(info, parentInfo).getModuleInfo().isVersionAdvertised());
+
+    // Test cases where current Component Info is null so it should inherit from parent.
+
+    // 5. Chain of versionAdvertised is: true (parent) -> null (current) => true
+    parentInfo.setVersionAdvertisedField(new Boolean(true));
+    parentInfo.setVersionAdvertised(true);
+    info.setVersionAdvertisedField(null);
+    assertEquals(true, resolveComponent(info, parentInfo).getModuleInfo().isVersionAdvertised());
+
+    // 6. Chain of versionAdvertised is: true (parent) -> inherit (current) => true
+    parentInfo.setVersionAdvertisedField(new Boolean(true));
+    parentInfo.setVersionAdvertised(true);
+    info.setVersionAdvertisedField(null);
+    assertEquals(true, resolveComponent(info, parentInfo).getModuleInfo().isVersionAdvertised());
+
+    // 7. Chain of versionAdvertised is: false (parent) -> null (current) => false
+    parentInfo.setVersionAdvertisedField(new Boolean(false));
+    parentInfo.setVersionAdvertised(false);
+    info.setVersionAdvertisedField(null);
+    assertEquals(false, resolveComponent(info, parentInfo).getModuleInfo().isVersionAdvertised());
+
+    // 8. Chain of versionAdvertised is: false (parent) -> inherit (current) => false
+    parentInfo.setVersionAdvertisedField(new Boolean(false));
+    parentInfo.setVersionAdvertised(false);
+    info.setVersionAdvertisedField(null);
+    assertEquals(false, resolveComponent(info, parentInfo).getModuleInfo().isVersionAdvertised());
+  }
+
   private List<ComponentInfo> createComponentInfo(int count){
     List<ComponentInfo> result = new ArrayList<ComponentInfo>();
     if(count > 0) {

ambari-server/src/test/resources/stacks/HDP/2.2.0/services/HDFS/metainfo.xml

@@ -31,7 +31,7 @@
 
         <component>
           <name>DATANODE</name>
-          <versionAdvertised>false</versionAdvertised>
+          <versionAdvertised>true</versionAdvertised>
         </component>
 
         <component>