ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tom Beerbower" <tbeerbo...@hortonworks.com>
Subject Re: Review Request 32414: Ability to set rack awareness
Date Tue, 24 Mar 2015 13:58:52 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message