[AMBARI-24464] Integrate Blueprints with the new MPackAdvisor API for Configuration Recommendations by benyoka · Pull Request #2259 · apache/ambari

benyoka · 2018-09-06T12:00:25Z

What changes were proposed in this pull request?

This is a preview patch for mpack advisor support for blueprints. Unit tests are being written at the moment.

Mpack advisor is the new default engine for configuration recommendation. It is possible to use the legacy stack advisor though by setting use.legacy.stackadvisor=true in ambari.properties.

How was this patch tested?

Tested by manual blueprint deployment. Unit tests are being written.

asfgit · 2018-09-06T12:42:14Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/3866/
Test FAILed.
Test FAILured.

rnettleton

I've taken an initial look, and the patch looks fine to me so far.

One the unit tests are available, I'll re-review.

Thanks.

adoroszlai · 2018-09-10T03:28:55Z

+      )
+    );
+
+    return null;


Shouldn't this method return the map created by the previous statement, instead of null?

I think this is from an intermediate commit.

adoroszlai · 2018-09-10T03:32:09Z

+  }
+
+  private Map<String, Set<String>> gatherHostGroupBindings(ClusterTopology clusterTopology) {
+    Map<String, Set<String>> hgBindngs = Maps.newHashMap();


Typo in hgBindngs, should be hgBindings.

adoroszlai · 2018-09-10T03:44:51Z

    return getProperty(MPACK_ADVISOR_SCRIPT);
  }

+  public boolean isUseLegacyStackAdvisor() {


The name shouldUseLegacyStackAdvisor would be more natural instead of isUseLegacyStackAdvisor. The latter may be OK for a JavaBean.

adoroszlai · 2018-09-10T03:48:13Z

   */
  Stream<ResolvedComponent> getComponents();

+  Map<String, Set<ResolvedComponent>> getComponentsByHostgroup();


Group with capital G in getComponentsByHostGroup would be more in line with other methods in this interface.

adoroszlai · 2018-09-10T03:57:00Z

+public class ExceptionUtils {
+
+  public interface ThrowingLambda<T extends Exception, R> {
+    R doIt() throws T;


Since T is never used (see ? extends in unchecked), I think we could get rid of this custom interface in favor of Callable.

adoroszlai · 2018-09-10T03:58:25Z

+    R doIt() throws T;
+  }
+
+  public static <R> R unchecked(ThrowingLambda<? extends Exception, R> throwingLambda) {


Please add Javadoc.

adoroszlai · 2018-09-10T04:00:33Z

+  /**
+   * Recommend configurations by the mpack advisor, then store the results in cluster topology.
+   * @param clusterTopology cluster topology instance
+   * @param userProvidedConfigurations User configurations of cluster provided in Blueprint + Cluster template


I think this doc (with the exception of "mpack") belongs to the interface being implemented.

adoroszlai · 2018-09-10T04:03:34Z

+  static
+  {
+    userContext = new HashMap<>();
+    userContext.put("operation", "ClusterCreate");


Can be replaced with an ImmutableMap, initialized at the field declaration?

adoroszlai · 2018-09-10T04:05:05Z

+  private static MpackAdvisorHelper mpackAdvisorHelper;
+
+  static final String RECOMMENDATION_FAILED = "Configuration recommendation failed.";
+  static final String INVALID_RESPONSE = "Configuration recommendation returned with invalid response.";


These constants could be extracted to the interface to avoid duplication.

adoroszlai · 2018-09-10T04:50:21Z

+      Set<Component> components = entry.getValue();
+      components.forEach( component -> {
+        Set<String> mpacksForComponent = getMpacksForComponent(component, componentNameToMpacks);
+        mpacksForComponent.forEach(mpack -> {


The way this class associates components to mpacks (and services in copyAndEnrichMpackInstances) is a bit redundant, since it's already performed by StackComponentResolver and the result is stored in ResolvedComponent instances. If some information is missing, I think it should be added there to avoid duplicated, and potentially different, implementations.

Good catch. I've rewritten the logic to leverage ResolvedComponent's.

asfgit · 2018-09-11T11:08:19Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/3906/
Test FAILed.
Test FAILured.

asfgit · 2018-09-11T21:30:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/3910/
Test FAILed.
Test FAILured.

adoroszlai · 2018-09-12T03:54:15Z

+    Map<StackId, Set<String>> mpackServices = topology.getComponents().collect(toMap(
+      ResolvedComponent::stackId,
+      comp -> ImmutableSet.of(comp.serviceInfo().getName()),
+      (set1, set2) -> ImmutableSet.copyOf(Sets.union(set1, set2))


Can be done less expensively (and perhaps more simply) by using built-ins:

Map<StackId, Set<String>> mpackServices = topology.getComponents().collect( groupingBy(ResolvedComponent::stackId, mapping(comp -> comp.serviceInfo().getName(), toSet())));

…nto AMBARI-24464-branch-feature-AMBARI-14714

benyoka · 2018-09-12T11:33:13Z

Looks like many test failures are due to this change. Looking into it.

asfgit · 2018-09-12T12:13:20Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/3919/
Test FAILed.
Test FAILured.

asfgit · 2018-09-12T13:22:35Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/3922/
Test FAILed.
Test FAILured.

adoroszlai · 2018-09-12T14:10:45Z

Thanks for the changes. Please make sure to fix the remaining unit tests, if possible.

asfgit · 2018-09-12T15:07:43Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/3925/
Test FAILed.
Test FAILured.

benyoka · 2018-09-12T17:16:47Z

retest this please

asfgit · 2018-09-12T17:56:38Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/3929/
Test FAILed.
Test FAILured.

benyoka · 2018-09-13T09:37:59Z

retest this please

adoroszlai · 2018-09-13T09:55:08Z

Latest build failed at Python tests, and is not caused by this change, happens on the base branch, too.

FAIL: test_get_stack_feature (TestStackFeature.TestStackFeature)

asfgit · 2018-09-13T10:19:18Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/3933/
Test FAILed.
Test FAILured.

Balazs Bence Sari added 4 commits August 29, 2018 11:00

AMBARI-24464 blueprint mpack advisor WIP (benyoka)

8da6d91

AMBARI-24464 blueprint mpack advisor WIP apache#2 (benyoka)

57d5904

AMBARI-24464 copy config to service instances

eefed12

AMBARI-24464 copy config to service instances reverted (benyoka)

9bff6f8

benyoka requested review from adoroszlai, rnettleton and swapanshridhar September 6, 2018 12:00

rnettleton reviewed Sep 7, 2018

View reviewed changes

adoroszlai changed the title ~~Ambari 24464 branch feature ambari 14714~~ [AMBARI-24464] Integrate Blueprints with the new MPackAdvisor API for Configuration Recommendations Sep 10, 2018

adoroszlai assigned benyoka Sep 10, 2018

adoroszlai added the blueprint label Sep 10, 2018

adoroszlai suggested changes Sep 10, 2018

View reviewed changes

AMBARI-24464 fix review comments + unit test (benyoka)

10de226

Balazs Bence Sari added 2 commits September 11, 2018 13:31

AMBARI-24464 fix some more review comments (benyoka)

d5828d0

AMBARI-24464 fix some more review comments apache#2 (benyoka)

79ea15f

adoroszlai reviewed Sep 12, 2018

View reviewed changes

Balazs Bence Sari added 2 commits September 12, 2018 12:42

AMBARI-24464 fix some more review comments apache#3 (benyoka)

4b5bdc9

Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…

c70b788

…nto AMBARI-24464-branch-feature-AMBARI-14714

AMBARI-24464 fix failing unit tests (benyoka)

bb3e81d

adoroszlai approved these changes Sep 12, 2018

View reviewed changes

AMBARI-24464 fix failing unit tests apache#2 (benyoka)

3dcc269

rnettleton approved these changes Sep 13, 2018

View reviewed changes

benyoka merged commit 1edb62b into apache:branch-feature-AMBARI-14714 Sep 14, 2018

Conversation

benyoka commented Sep 6, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

asfgit commented Sep 6, 2018

Uh oh!

rnettleton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benyoka Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Sep 11, 2018

Uh oh!

asfgit commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benyoka commented Sep 12, 2018

Uh oh!

asfgit commented Sep 12, 2018

Uh oh!

asfgit commented Sep 12, 2018

Uh oh!

adoroszlai commented Sep 12, 2018

Uh oh!

asfgit commented Sep 12, 2018

Uh oh!

benyoka commented Sep 12, 2018

Uh oh!

asfgit commented Sep 12, 2018

Uh oh!

benyoka commented Sep 13, 2018

Uh oh!

adoroszlai commented Sep 13, 2018

Uh oh!

asfgit commented Sep 13, 2018

Uh oh!

Reviewers

Assignees

benyoka Sep 11, 2018 •

edited

Loading