ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Nettleton" <rnettle...@hortonworks.com>
Subject Re: Review Request 39481: Basic StackAdvisor support for blueprint (configurations)
Date Fri, 23 Oct 2015 16:56:14 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39481/#review103776
-----------------------------------------------------------


Overall, I think the changes here look great.

There are a few minor issues that need to be resolved.

As I see it, there is only one major issue to be resolved.  I think that the approach towards
filtering out the stack defaults should be reconsidered.  My comments mention that the Stack
class provides a way to obtain the Configuration object associated with the Stack.  If this
Configuration instance contains all stack defaults for the full Stack hierarchy (from the
current version back), then I think this would be a more straightforward way to filter out
the stack defaults vs. user-supplied configuration.  

I'm going to hold off on a +1 until this issue is re-visited, but I think the patch is in
great shape.  I just think that it may be simpler and easier to maintain if we can skip calling
the Configuration.getParent() method at multiple levels, since the underlying implementation
may change someday.  

Thanks for the great work!


ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java
(line 50)
<https://reviews.apache.org/r/39481/#comment161856>

    Minor issue: 
    
    It appears that these strings are meant for the error-handling exceptions below, but they
don't appear to be used.
    
    If my understanding is correct, then I'd recommend the following minor changes:
    
    1. Make these two static strings "final" as well
    2. Update the exceptions being created in adviseConfiguration() to use these constants.



ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java
(line 85)
<https://reviews.apache.org/r/39481/#comment161858>

    How will the new dynamic model, introduced in 2.1.0, be supported here?  
    
    Since a Cluster Creation template can specify a predicate to use for matching host groups
to servers as they come online, there needs to be a way to obtain recommendations for hosts
as they are registered.  
    
    This question may be answered later in the code, so I'll remove this issue if that is
the case.



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
(line 402)
<https://reviews.apache.org/r/39481/#comment161860>

    Minor issue:
    
    I'd recommend using a left-to-right pattern for the conditionals.
    
    Example:
    
    if (configRecommendationStrategy.equals(ConfigRecommendationStrategy.ONLY_STACK_DEFAULTS_APPLY))



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
(line 410)
<https://reviews.apache.org/r/39481/#comment161863>

    Minor issue: 
    
    It might be helpful to add the name of the config type being updated to this log statement.



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
(line 420)
<https://reviews.apache.org/r/39481/#comment161866>

    Minor javadoc issue:
    
    Please change
    
    "what is not found..." 
    
    to
    
    "that is not found..."



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
(line 430)
<https://reviews.apache.org/r/39481/#comment161865>

    I think there might be a more straightforward way to do this, rather than rely on the
Configuration hierarchy.  
    
    There might be cases in which using the parent 2 levels up might not work (host-group
level configuration, for example).
    
    The following class for Stack processing:
    
    org.apache.ambari.server.controller.internal.Stack
    
    includes a "getConfiguration()" method, that appears to return a Configuration instance
that only includes the Stack defaults for that Stack version (plus any defaults in the Stack
hierarchy.  
    
    I think this method should be modified to use the Stack.getConfiguration() method, and
then compare the cluster config with the stack config, in order to make the distinction between
properties supplied by the Blueprint/Cluster Creation Template and stack defaults.



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
(line 478)
<https://reviews.apache.org/r/39481/#comment161867>

    Minor issue:
    
    Please reverse the ordering of the conditional here.
    
    Example:  if (valueAttrEntry.getValue().getDelete().equalsIgnoreCase("true"))



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ProvisionClusterRequest.java
(line 108)
<https://reviews.apache.org/r/39481/#comment161868>

    Minor issue:
    
    It might be good to declare this strategy as a "final" variable, since this strategy should
be an invariant of the cluster deployment.



ambari-server/src/main/java/org/apache/ambari/server/topology/ConfigRecommendationStrategy.java
(line 34)
<https://reviews.apache.org/r/39481/#comment161876>

    Minor javadoc change:
    
    Please change:
    
    "but no for..."
    
    to 
    
    "but not for..."



ambari-server/src/main/java/org/apache/ambari/server/topology/TopologyManager.java (line 116)
<https://reviews.apache.org/r/39481/#comment161877>

    Minor issue:
    
    As in my comments above, I'd recommend reversing the ordering of this conditional, for
the sake of readability.  
    
    Thanks.



ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
(line 5266)
<https://reviews.apache.org/r/39481/#comment161888>

    These tests look great.  
    
    I'd make a small request here:
    
    Maybe add a unit test to specifically test out the "NEVER_APPLY" case as well.
    
    While I realize that most other unit tests will verify this, I think it helps to explicitly
test this mode as well. That will help with maintaining this code in the future, especially
if new strategies are added, or if we decide to change the default strategy in the future.


- Robert Nettleton


On Oct. 20, 2015, 4:37 p.m., Oliver Szabo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39481/
> -----------------------------------------------------------
> 
> (Updated Oct. 20, 2015, 4:37 p.m.)
> 
> 
> Review request for Ambari, Robert Nettleton and Sumit Mohanty.
> 
> 
> Bugs: AMBARI-13487
>     https://issues.apache.org/jira/browse/AMBARI-13487
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> During update configuration for cluster creation, configurations are upgraded based on
response of the StackAdvisor.
> The advised configurations will be added to cluster topology, and used during BlueprintConfigurationProcessor#doUpdateForClusterCreate().
The advised configurations filtered out based on ConfigRecommendationStrategy, which can be
set in the Cluster Creation Template, e.g.:
> {
>   "blueprint" : "multi-node-conf-yarn",
>   "default_password" : "secret",
>   "config_recommendation_strategy" : "ONLY_STACK_DEFAULTS_APPLY",
>     "host_groups": [
>         {
>           "hosts": [
>               { "fqdn": "c6402.ambari.apache.org" }
>           ],
>           "name": "master",
>           "configurations" : [ ]
>         },
>         {
>           "hosts": [
>               { "fqdn": "c6403.ambari.apache.org" }
>           ],
>           "name": "slave_1",
>           "configurations" : [ ]
>         }
>     ],
>   "Clusters" : {"cluster_name":"cluster_name"}
> }
> 
> I added the configuration modifications BEFORE the basic updates happen in BlueprintConfigurationProcessor.
> As I see stackAdvisor not supports host group scoped configurations, so as my changes.
I made a new AdvisedConfiguration which has a Map field for properties, this field can be
a Configuration object in the future (if property attributes will be supported..because they
are not supported too)
> 
> question: is that the correct way to geather stack defaults in BlueprintConfigurationProcessor#doFilterStackDefaults()
?
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 8f3869f

>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
18fca8a 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java
e2f132e 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ProvisionClusterRequest.java
4a906b1 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/AdvisedConfiguration.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/ClusterConfigurationRequest.java
1ebde17 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/ClusterTopology.java
284a913 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/ClusterTopologyImpl.java
5b716ae 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/ConfigRecommendationStrategy.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/TopologyManager.java
64be609 
>   ambari-server/src/test/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessorTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
34b72b6 
>   ambari-server/src/test/java/org/apache/ambari/server/topology/TopologyManagerTest.java
8eeb54c 
> 
> Diff: https://reviews.apache.org/r/39481/diff/
> 
> 
> Testing
> -------
> 
> Unit tests passed:
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 01:30 h
> [INFO] Finished at: 2015-10-20T16:23:54+00:00
> [INFO] Final Memory: 48M/687M
> [INFO] -----------------------------------------------------------------------
> 
> Functional tests: Manually
> (added "config_recommendation_strategy" field to the cluster creation template: tried
4 variations: field is missing, and set the field to NEVER_APPLY, ALWAYS_APPLY and ONLY_STACK_DEFAULTS_APPLY)
> 
> 
> Thanks,
> 
> Oliver Szabo
> 
>


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