You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Robert Nettleton <rn...@hortonworks.com> on 2014/10/22 02:57:02 UTC
Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/
-----------------------------------------------------------
Review request for Ambari, John Speidel and Nate Cole.
Repository: ambari
Description
-------
This patch implements a fix for bug: AMBARI-7738.
The Blueprints configuration processor was not properly handling
the case of an optional service in an HDP 2.1 stack deployment.
The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
included in a Blueprint that references HDP 2.1. Posting the
Blueprint will work properly, but launching a cluster based
on this Blueprint will fail, with an error mentioning that
the APP_TIMELINE_SERVER is not included.
The APP_TIMELINE_SERVER has a cardinality of "0-1" for
HDP 2.1, meaning that 0 or 1 instances of this service
can be deployed. This cardinality was selected for HDP
2.1 because this service was considered a technical
preview for the HDP 2.1 stack.
While this particular Yarn component demonstrates the problem,
the issue is actually a bit more generic. The Blueprint
config processor is not currently aware of the cardinality
of each service being deployed, so the failure occurs when the
processor attempts to update configuration for a serivce that
doesn't exist in the proposed cluster.
This patch addresses the problem with the following changes:
- Moves two inner classes (Stack, Cardinality) to the
top-level, as package-level classes. These classes
offer useful processing features for the Blueprint
config processor, and are more useful as standalone
classes.
- Modifies the signatures of the PropertyUpdater interface,
any implementations of this interface, and the call stack
from the ClusterResourceProvider to the BlueprintConfiguration
processor, such that the parsed Stack object is available
to the PropertyUpdater instances.
- Modifies the implementation for the SingleHostTopologyUpdater
to query the Stack definition in the event that no hosts
are found when trying to update a property. If a cardinality
of 0 is found to be valid for that service, then the original
value for the property is returned. If a cardinality of 0
is not valid for this service, then an IllegalArgumentException
is thrown back to the caller, indicating that an error has
occurred (this was the previous behavior in both cases).
- Updates various existing unit tests to reflect this change.
- Adds new unit tests to verify the fix.
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 0b58c8d
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java a57bacb
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
Diff: https://reviews.apache.org/r/27006/diff/
Testing
-------
1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
2. Manually verified the fix against the trunk code.
3. Manually verified the fix against the 1.7.0 code.
Thanks,
Robert Nettleton
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by John Speidel <js...@hortonworks.com>.
> On Oct. 23, 2014, 3:07 p.m., John Speidel wrote:
> > Ship It!
Merged to 1.7.0 and trunk.
All unit tests pass in both branches.
Results :
Tests run: 2121, Failures: 0, Errors: 0, Skipped: 17
...
Total run:669
Total errors:0
Total failures:0
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57992
-----------------------------------------------------------
On Oct. 22, 2014, 9:44 p.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 9:44 p.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 10cc016
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java 260d043
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by John Speidel <js...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57992
-----------------------------------------------------------
Ship it!
Ship It!
- John Speidel
On Oct. 22, 2014, 9:44 p.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 9:44 p.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 10cc016
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java 260d043
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by Robert Nettleton <rn...@hortonworks.com>.
> On Oct. 23, 2014, 2:24 a.m., Alejandro Fernandez wrote:
> >
Thanks for the review.
> On Oct. 23, 2014, 2:24 a.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java, line 35
> > <https://reviews.apache.org/r/27006/diff/2/?file=729115#file729115line35>
> >
> > Should avoid attempting to parse things like "+" or "a+".
> > Is there a need to do a case insensitive comparison with "ALL"?
As I mentioned in the summary, this class already existed in the Ambari Blueprints codebase. This patch just moves the class to the top-level so that it can be more easily re-used. I have not modified this class, other than the refactoring required to move it.
The "+" character is already used as a possible value for the cardinality of a given HDP component. This is not something we can change at this point, since the Stack definitions use this syntax.
Regarding the "ALL" comparison, this should be case sensitive.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57960
-----------------------------------------------------------
On Oct. 22, 2014, 9:44 p.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 9:44 p.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 10cc016
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java 260d043
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by Alejandro Fernandez <af...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57960
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java
<https://reviews.apache.org/r/27006/#comment98871>
Should avoid attempting to parse things like "+" or "a+".
Is there a need to do a case insensitive comparison with "ALL"?
- Alejandro Fernandez
On Oct. 22, 2014, 9:44 p.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 9:44 p.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 10cc016
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java 260d043
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by Robert Nettleton <rn...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/
-----------------------------------------------------------
(Updated Oct. 22, 2014, 9:44 p.m.)
Review request for Ambari, John Speidel and Nate Cole.
Changes
-------
Uploaded new patch, implemented suggestions from reviewer.
Repository: ambari
Description
-------
This patch implements a fix for bug: AMBARI-7738.
The Blueprints configuration processor was not properly handling
the case of an optional service in an HDP 2.1 stack deployment.
The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
included in a Blueprint that references HDP 2.1. Posting the
Blueprint will work properly, but launching a cluster based
on this Blueprint will fail, with an error mentioning that
the APP_TIMELINE_SERVER is not included.
The APP_TIMELINE_SERVER has a cardinality of "0-1" for
HDP 2.1, meaning that 0 or 1 instances of this service
can be deployed. This cardinality was selected for HDP
2.1 because this service was considered a technical
preview for the HDP 2.1 stack.
While this particular Yarn component demonstrates the problem,
the issue is actually a bit more generic. The Blueprint
config processor is not currently aware of the cardinality
of each service being deployed, so the failure occurs when the
processor attempts to update configuration for a serivce that
doesn't exist in the proposed cluster.
This patch addresses the problem with the following changes:
- Moves two inner classes (Stack, Cardinality) to the
top-level, as package-level classes. These classes
offer useful processing features for the Blueprint
config processor, and are more useful as standalone
classes.
- Modifies the signatures of the PropertyUpdater interface,
any implementations of this interface, and the call stack
from the ClusterResourceProvider to the BlueprintConfiguration
processor, such that the parsed Stack object is available
to the PropertyUpdater instances.
- Modifies the implementation for the SingleHostTopologyUpdater
to query the Stack definition in the event that no hosts
are found when trying to update a property. If a cardinality
of 0 is found to be valid for that service, then the original
value for the property is returned. If a cardinality of 0
is not valid for this service, then an IllegalArgumentException
is thrown back to the caller, indicating that an error has
occurred (this was the previous behavior in both cases).
- Updates various existing unit tests to reflect this change.
- Adds new unit tests to verify the fix.
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 10cc016
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java 260d043
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
Diff: https://reviews.apache.org/r/27006/diff/
Testing
-------
1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
2. Manually verified the fix against the trunk code.
3. Manually verified the fix against the 1.7.0 code.
Thanks,
Robert Nettleton
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by Robert Nettleton <rn...@hortonworks.com>.
> On Oct. 22, 2014, 12:46 p.m., Nate Cole wrote:
> >
Thanks for the review!
> On Oct. 22, 2014, 12:46 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java, line 40
> > <https://reviews.apache.org/r/27006/diff/1/?file=728468#file728468line40>
> >
> > Never understood why * was never used, but ok.
I'm not exactly sure, since I've basically moved this existing code to a top-level class, and have left the impl unchanged. Since this cardinality value is used in the stacks, it might be difficult to change without breaking things.
> On Oct. 22, 2014, 12:46 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java, lines 40-47
> > <https://reviews.apache.org/r/27006/diff/1/?file=728470#file728470line40>
> >
> > We have a lot of Stack representations already. Can one of them be augmented?
It's possible, but this was the only Stack parsing class I could find that included support for Cardinality. In the future, I'm definitely willing to use a more general class if it becomes available, but would rather stick with this one for the 1.7.0 timeframe. This particular class has the advantage of handling all the details of the Stack query API, so I find it a bit simpler to use.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57776
-----------------------------------------------------------
On Oct. 22, 2014, 12:57 a.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 12:57 a.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 0b58c8d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java a57bacb
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by John Speidel <js...@hortonworks.com>.
> On Oct. 22, 2014, 12:46 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java, lines 40-47
> > <https://reviews.apache.org/r/27006/diff/1/?file=728470#file728470line40>
> >
> > We have a lot of Stack representations already. Can one of them be augmented?
>
> Robert Nettleton wrote:
> It's possible, but this was the only Stack parsing class I could find that included support for Cardinality. In the future, I'm definitely willing to use a more general class if it becomes available, but would rather stick with this one for the 1.7.0 timeframe. This particular class has the advantage of handling all the details of the Stack query API, so I find it a bit simpler to use.
I agree that the various Stack implementations should at some time be consilidated. This class is basically a wrapper around the currently existing stack api's in AmbariManagementController and AmbariMetaInfo. Those api's are a mess so this was created to abstract the BP code from the details of dealing with these existing api's. As a note, I my currenty work, I have done a lot of cleanup of stack related code and introduced a StackManager implementation which should help in the consolidation effort.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57776
-----------------------------------------------------------
On Oct. 22, 2014, 12:57 a.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 12:57 a.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 0b58c8d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java a57bacb
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by John Speidel <js...@hortonworks.com>.
> On Oct. 22, 2014, 12:46 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java, line 40
> > <https://reviews.apache.org/r/27006/diff/1/?file=728468#file728468line40>
> >
> > Never understood why * was never used, but ok.
>
> Robert Nettleton wrote:
> I'm not exactly sure, since I've basically moved this existing code to a top-level class, and have left the impl unchanged. Since this cardinality value is used in the stacks, it might be difficult to change without breaking things.
The reason that * wasn't used is that it means any value is good. 'ALL' means that the component MUST be on every host.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57776
-----------------------------------------------------------
On Oct. 22, 2014, 12:57 a.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 12:57 a.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 0b58c8d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java a57bacb
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by Nate Cole <nc...@hortonworks.com>.
> On Oct. 22, 2014, 8:46 a.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java, line 40
> > <https://reviews.apache.org/r/27006/diff/1/?file=728468#file728468line40>
> >
> > Never understood why * was never used, but ok.
>
> Robert Nettleton wrote:
> I'm not exactly sure, since I've basically moved this existing code to a top-level class, and have left the impl unchanged. Since this cardinality value is used in the stacks, it might be difficult to change without breaking things.
>
> John Speidel wrote:
> The reason that * wasn't used is that it means any value is good. 'ALL' means that the component MUST be on every host.
Got it - thanks for the clarify!
- Nate
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57776
-----------------------------------------------------------
On Oct. 21, 2014, 8:57 p.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 8:57 p.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 0b58c8d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java a57bacb
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57776
-----------------------------------------------------------
Ship it!
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java
<https://reviews.apache.org/r/27006/#comment98660>
I would actually stand and applaud a 2 billion node cluster :)
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java
<https://reviews.apache.org/r/27006/#comment98661>
Never understood why * was never used, but ok.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
<https://reviews.apache.org/r/27006/#comment98658>
We have a lot of Stack representations already. Can one of them be augmented?
- Nate Cole
On Oct. 21, 2014, 8:57 p.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 8:57 p.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 0b58c8d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java a57bacb
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by Robert Nettleton <rn...@hortonworks.com>.
> On Oct. 22, 2014, 6:03 p.m., John Speidel wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java, line 497
> > <https://reviews.apache.org/r/27006/diff/1/?file=728467#file728467line497>
> >
> > minor nit
> >
> > all param descriptions should be vertically aligned
Fixed.
> On Oct. 22, 2014, 6:03 p.m., John Speidel wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java, line 558
> > <https://reviews.apache.org/r/27006/diff/1/?file=728467#file728467line558>
> >
> > In the case where there are multiple hostgroup mappings, you are still returning the original value. I would think that you should also check that matchingGroups.size() == 0. If we get here and there are multiple matching groups, this is still an error condition even if the 0 is a valid cardinality for the component.
Fixed. I've also added a new unit test to catch this case, where the number of matching host groups is > 0, to verify that the exception is still thrown in this case, regardless of the cardinality value.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57830
-----------------------------------------------------------
On Oct. 22, 2014, 9:44 p.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 9:44 p.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 10cc016
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java 260d043
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>
Re: Review Request 27006: Adding handler code for components with 0-1
cardinality in Blueprint configuration processor
Posted by John Speidel <js...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27006/#review57830
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
<https://reviews.apache.org/r/27006/#comment98706>
Need to add stackDefinition param to javadoc
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
<https://reviews.apache.org/r/27006/#comment98702>
minor nit
all param descriptions should be vertically aligned
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
<https://reviews.apache.org/r/27006/#comment98705>
Need to add stackDefinition param to javadoc
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
<https://reviews.apache.org/r/27006/#comment98714>
In the case where there are multiple hostgroup mappings, you are still returning the original value. I would think that you should also check that matchingGroups.size() == 0. If we get here and there are multiple matching groups, this is still an error condition even if the 0 is a valid cardinality for the component.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
<https://reviews.apache.org/r/27006/#comment98707>
need to add stackDefinition param to javadoc
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
<https://reviews.apache.org/r/27006/#comment98708>
need to add stackDefinition param to javadoc
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
<https://reviews.apache.org/r/27006/#comment98709>
need to add stackDefinition param to javadoc
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
<https://reviews.apache.org/r/27006/#comment98710>
need to add stackDefinition param to javadoc
- John Speidel
On Oct. 22, 2014, 12:57 a.m., Robert Nettleton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27006/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 12:57 a.m.)
>
>
> Review request for Ambari, John Speidel and Nate Cole.
>
>
> Repository: ambari
>
>
> Description
> -------
>
> This patch implements a fix for bug: AMBARI-7738.
>
> The Blueprints configuration processor was not properly handling
> the case of an optional service in an HDP 2.1 stack deployment.
>
> The problem occurs when the Yarn "APP_TIMELINE_SERVER" is not
> included in a Blueprint that references HDP 2.1. Posting the
> Blueprint will work properly, but launching a cluster based
> on this Blueprint will fail, with an error mentioning that
> the APP_TIMELINE_SERVER is not included.
>
> The APP_TIMELINE_SERVER has a cardinality of "0-1" for
> HDP 2.1, meaning that 0 or 1 instances of this service
> can be deployed. This cardinality was selected for HDP
> 2.1 because this service was considered a technical
> preview for the HDP 2.1 stack.
>
> While this particular Yarn component demonstrates the problem,
> the issue is actually a bit more generic. The Blueprint
> config processor is not currently aware of the cardinality
> of each service being deployed, so the failure occurs when the
> processor attempts to update configuration for a serivce that
> doesn't exist in the proposed cluster.
>
> This patch addresses the problem with the following changes:
>
> - Moves two inner classes (Stack, Cardinality) to the
> top-level, as package-level classes. These classes
> offer useful processing features for the Blueprint
> config processor, and are more useful as standalone
> classes.
>
> - Modifies the signatures of the PropertyUpdater interface,
> any implementations of this interface, and the call stack
> from the ClusterResourceProvider to the BlueprintConfiguration
> processor, such that the parsed Stack object is available
> to the PropertyUpdater instances.
>
> - Modifies the implementation for the SingleHostTopologyUpdater
> to query the Stack definition in the event that no hosts
> are found when trying to update a property. If a cardinality
> of 0 is found to be valid for that service, then the original
> value for the property is returned. If a cardinality of 0
> is not valid for this service, then an IllegalArgumentException
> is thrown back to the caller, indicating that an error has
> occurred (this was the previous behavior in both cases).
>
> - Updates various existing unit tests to reflect this change.
>
> - Adds new unit tests to verify the fix.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessor.java abd22a4
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 0b58c8d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Cardinality.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java 090eb92
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BaseBlueprintProcessorTest.java be5aea8
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java a57bacb
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java c1185ae
>
> Diff: https://reviews.apache.org/r/27006/diff/
>
>
> Testing
> -------
>
> 1. Ran the ambari-server unit test suite (both trunk and 1.7.0), and all unit tests are passing with this patch applied.
> 2. Manually verified the fix against the trunk code.
> 3. Manually verified the fix against the 1.7.0 code.
>
>
> Thanks,
>
> Robert Nettleton
>
>