> On March 24, 2015, 1:24 p.m., Jonathan Hurley wrote: > > Some minor nits and a few general questions just asking if the behavior is > > expected.
Thanks for reviewing! > On March 24, 2015, 1:24 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/services/AmbariMetaInfo.java, > > line 529 > > <https://reviews.apache.org/r/32414/diff/1/?file=903401#file903401line529> > > > > Any reason this is a Boolean object? Seems like it could cause NPEs in > > when used by other areas of code. To be honest, this was existing and I just left the type alone, but I think that there is a reason that the primitive type was not used. In ServiceModule, there is this code ... if (serviceInfo.isRestartRequiredAfterChange() == null) { serviceInfo.setRestartRequiredAfterChange(parent.isRestartRequiredAfterChange()); } So, null indicates that the value is not set at this level and should be taken from the parent. > On March 24, 2015, 1:24 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/services/AmbariMetaInfo.java, > > line 557 > > <https://reviews.apache.org/r/32414/diff/1/?file=903401#file903401line557> > > > > Same as above; any reason this can't be a primitive to avoid NPEs and > > null checks? Same as above, I basically cut and pasted the code, but it looks like there is a valid reason for using the big B Boolean. > On March 24, 2015, 1:24 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, > > line 633 > > <https://reviews.apache.org/r/32414/diff/1/?file=903403#file903403line633> > > > > Slightly confusing; the first time I read this, I thought these were > > the services that need a restart. But they are just the ones that are rack > > sensitive. Can we rename this Set to something like `rackSenstiveServices` Sure, that makes more sense. > On March 24, 2015, 1:24 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/resources/common-services/HDFS/2.1.0.2.0/package/scripts/hdfs_namenode.py, > > line 21 > > <https://reviews.apache.org/r/32414/diff/1/?file=903411#file903411line21> > > > > Correct global imports. Yes, I'll fix it. > On March 24, 2015, 1:24 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/resources/common-services/HDFS/2.1.0.2.0/package/scripts/hdfs_namenode.py, > > line 69 > > <https://reviews.apache.org/r/32414/diff/1/?file=903411#file903411line69> > > > > If I'm reading this right, it's going to generate a new topology > > script/mapping every time NN is started. Is that what we want even if the > > environment is not rack aware? Yes, we may needlessly regenerate the mapping and copy the script on NN restart if the rack info hasn't actually changed. I think that the cost of determining whether or not it has changed is probably more than just doing it given how infrequently a NN restart occurs. - Tom ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32414/#review77558 ----------------------------------------------------------- On March 23, 2015, 8:35 p.m., Tom Beerbower wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32414/ > ----------------------------------------------------------- > > (Updated March 23, 2015, 8:35 p.m.) > > > Review request for Ambari, Jonathan Hurley, John Speidel, and Nate Cole. > > > Bugs: AMBARI-6466 > https://issues.apache.org/jira/browse/AMBARI-6466 > > > Repository: ambari > > > Description > ------- > > 1. Generate topology script used by Hadoop to determine the rack location of > nodes. > 2. Update core-site.xml to point to script. > 3. Provide mechanism for marking services that need restart when host rack > info changes. > 4. React to host resource rack changes to ... > a. mark services needing restart. > b. update host/rack data for topology script > > > Diffs > ----- > > ambari-agent/src/main/python/ambari_agent/CustomServiceOrchestrator.py > 8c21503 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/AmbariMetaInfo.java > 982f10f > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementController.java > 6d35a36 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > 98390fd > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostResourceProvider.java > 3a359e5 > > ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceModule.java > ab1ede5 > ambari-server/src/main/java/org/apache/ambari/server/state/ServiceInfo.java > 939a496 > ambari-server/src/main/java/org/apache/ambari/server/utils/StageUtils.java > de84f35 > > ambari-server/src/main/resources/common-services/HDFS/2.1.0.2.0/configuration/core-site.xml > c0cd6b3 > > ambari-server/src/main/resources/common-services/HDFS/2.1.0.2.0/metainfo.xml > 916d9b0 > > ambari-server/src/main/resources/common-services/HDFS/2.1.0.2.0/package/files/topology_script.py > PRE-CREATION > > ambari-server/src/main/resources/common-services/HDFS/2.1.0.2.0/package/scripts/hdfs_namenode.py > 615dd54 > > ambari-server/src/main/resources/common-services/HDFS/2.1.0.2.0/package/scripts/params.py > d20e8e0 > > ambari-server/src/main/resources/common-services/HDFS/2.1.0.2.0/package/templates/topology_mappings.data.j2 > PRE-CREATION > > ambari-server/src/main/resources/common-services/YARN/2.1.0.2.0/metainfo.xml > 974165c > > ambari-server/src/test/java/org/apache/ambari/server/api/services/AmbariMetaInfoTest.java > 8f7c199 > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java > 208218c > > ambari-server/src/test/java/org/apache/ambari/server/state/ServiceInfoTest.java > PRE-CREATION > ambari-server/src/test/python/stacks/2.0.6/HDFS/test_namenode.py 1919958 > ambari-server/src/test/python/stacks/2.0.6/configs/default.json d1701bd > > ambari-server/src/test/python/stacks/2.0.6/configs/ha_bootstrap_active_node.json > b3e61bc > > ambari-server/src/test/python/stacks/2.0.6/configs/ha_bootstrap_standby_node.json > 4bbb0f5 > ambari-server/src/test/python/stacks/2.0.6/configs/ha_default.json 652ca7b > ambari-server/src/test/python/stacks/2.0.6/configs/ha_secured.json 6c06f35 > ambari-server/src/test/python/stacks/2.0.6/configs/secured.json b0b0c04 > > ambari-server/src/test/resources/stacks/HDP/2.0.7/services/HDFS/metainfo.xml > 69f13e9 > > Diff: https://reviews.apache.org/r/32414/diff/ > > > Testing > ------- > > Manual testing to verify that topology script is configured and available, > services (HDFS, MAPRED2) are marked for restart when a change is made to host > rack information, and topology script can be invoked to get updated rack > information. > > New unit tests added. > > All unit tests pass. > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 33:36 min > [INFO] Finished at: 2015-03-23T15:47:10-04:00 > [INFO] Final Memory: 43M/600M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Tom Beerbower > >
