[AMBARI-24162] Support for cluster-settings (remove cluster-env) by benyoka · Pull Request #1627 · apache/ambari

benyoka · 2018-06-26T15:52:26Z

What changes were proposed in this pull request?

Properties from cluster-env that are recognized as cluster settings are copied/moved (copied in case ambari code still relies on cluster-env for a property) to the setting object.
cluster settings coming from blueprint are processed during initial resource creation
fixed the handling of the setting object in the blueprint (it was not saved)
retry related settings are based on cluster settings instead of cluster-env

How was this patch tested?

tested cluster installation manually
wrote new unit tests
unit test run results:

Tests run: 4810, Failures: 12, Errors: 60, Skipped: 97
vs
Tests run: 4809, Failures: 13, Errors: 63, Skipped: 97 on feature branch

…benyoka)

…eature-AMBARI-14714

asfgit · 2018-06-26T17:01:25Z

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

rnettleton

The patch looks fine to me, just a few questions/clarifications based on this change.

rnettleton · 2018-06-26T21:12:01Z

    }
  }

-  /**


How will existing Blueprints that set these properties be handled?

We should make sure that if these properties are set in the older location ("cluster-env") then the Blueprint processor should move these over to "cluster-settings".

The logic is the following:

all properties in cluster-env which are recognized as cluster settings are copied from cluster-env to cluster settings.

recognition is based on the default cluster settings. It is stored in cluster-settings.xml and is exposed by a read only API. If a default setting is found for a cluster-env property, the property will be treated as cluster setting.

Legacy Ambari code may still use cluster-env to get some of these properties, so as a temporary workaround the properties will remain in cluster-env too (in addition to being copied to cluster settings). Exceptions are the cases where migration has happened; such case is the retry related settings.

Users are safe the continue using cluster-env with blueprints; the properties will be automatically copied to cluster settings (provided they are recognized cluster setting properties based on the logic above)

Handling recovery related settings in the BlueprintConfigurationProcessor is no more necessary as there are defaults defined in cluster-settings.xml for these properties.

rnettleton · 2018-06-26T21:52:00Z

    topologyManager.provisionCluster(request, "{}");
  }
+
+  private static ServiceResponse service(String serviceName, long serviceId) {


Maybe this method should be moved into a utility class, since it appears to be repeated in several test cases.

benyoka · 2018-06-27T10:22:32Z

Hey @rnettleton and @jayush

This PR enables users to send cluster settings with blueprints in two ways:

They can be defined as properties in cluster-env (the legacy way)
They can be defined in the setting part in the blueprint in a new section called cluster_settings. In addition, the recovery_settings section is also treated as cluster settings. (the proposed new way)

Please let me know if you are fine with the syntax. Here is an example:

...
"settings" : [
    "cluster_settings": [
       {
           "command_retry_enabled": "true",
           "commands_to_retry": "INSTALL,START,STOP",
           "command_retry_max_time_in_sec": "500"
       }
    ]
    "deployment_settings": [
      {"skip_failure":"true"}
    ],
    "repository_settings":[
       ...
    ],
    "recovery_settings":[
      {"recovery_enabled":"true"}
    ],
    "service_settings":[
      {
        "name":"SERVICE_ONE",
        "recovery_enabled":"true"
      },
      {
        "name":"SERVICE_TWO",
        "recovery_enabled":"true"
      }
    ],
    "component_settings":[
        ...
    ]
  ],
...

The current settings structure is overly granular for cluster settings (it defines a set of maps for each section), so there is some flexibility: cluster settings can be set both as a single map in the set (see the example above) or a set of maps like this:

...
    "cluster_settings": [
       { "command_retry_enabled": "true" },
       { "commands_to_retry": "INSTALL,START,STOP" },
       { "command_retry_max_time_in_sec": "500" }
    ]
...

adoroszlai

LGTM, but please consider some opportunities below to simplify parts of the code.

adoroszlai · 2018-06-27T12:31:33Z

-      cluster.addClusterSetting(p.getName(), p.getValue());
+    Map<String, String> defaultClusterSettings =
+      getController().getAmbariMetaInfo().getClusterProperties().stream()
+        .collect(toMap(p -> p.getName(), p -> p.getValue()));


Lambda can be simplified to method reference.

adoroszlai · 2018-06-27T12:45:53Z

+
+    Map<String, Object> settingProperties = properties.entrySet().stream()
+      .filter(entry -> SETTING_PROPERTY_ID.equals(entry.getKey()) || entry.getKey().startsWith(SETTING_PROPERTY_ID + "/"))
+      .collect(toMap(entry -> entry.getKey(), entry -> entry.getValue()));


Lambda can be simplified to method reference.

adoroszlai · 2018-06-27T12:49:34Z

-  private static void setRetryConfiguration(Configuration configuration, Set<String> configTypesUpdated) {
-    boolean wasUpdated = false;
-
-    if (configuration.getPropertyValue(CLUSTER_ENV_CONFIG_TYPE_NAME, COMMAND_RETRY_ENABLED_PROPERTY_NAME) == null) {


Can you please also remove leftover unused constants?

adoroszlai · 2018-06-27T13:03:00Z

+   */
+  private void adjustTopology() {
+    Set<PropertyInfo> clusterProperties = AmbariContext.getController().getAmbariMetaInfo().getClusterProperties();
+    Set<String> clusterSettingPropertyNames = clusterProperties.stream().map(prop -> prop.getName()).collect(toSet());


Lambda can be simplified to method reference.

adoroszlai · 2018-06-27T13:09:50Z

+      ImmutableSet.of(SETTING_NAME_CLUSTER_SETTINGS, SETTING_NAME_RECOVERY_SETTINGS);
+    return settingsToExtract.stream()
+      .flatMap( settingCategory ->
+        Optional.ofNullable(properties.get(settingCategory)).orElseGet(() -> emptySet()).stream())


Optional.ofNullable(properties.get(settingCategory)).orElseGet(() -> emptySet())

can be simplified to

properties.getOrDefault(settingCategory, emptySet())

adoroszlai · 2018-06-27T13:10:07Z

+      .flatMap( settingCategory ->
+        Optional.ofNullable(properties.get(settingCategory)).orElseGet(() -> emptySet()).stream())
+      .flatMap( map -> map.entrySet().stream() )
+      .collect(toMap(e -> e.getKey(), e -> e.getValue()));


Lambda can be simplified to method reference.

adoroszlai · 2018-06-27T13:26:33Z

+    Set<String> clusterSettingPropertyNames = clusterProperties.stream().map(prop -> prop.getName()).collect(toSet());
+    Map<String, String> clusterEnv = Optional
+      .ofNullable(configuration.getFullProperties().get(ConfigHelper.CLUSTER_ENV))
+      .orElse(ImmutableMap.of());


Can be simplified using getOrDefault.

adoroszlai · 2018-06-27T13:55:22Z

+            Setting.SETTING_NAME_CLUSTER_SETTINGS,
+            __ -> Sets.newHashSet(Maps.newHashMap()))
+          .iterator().next()
+          .put(entry.getKey(), entry.getValue());


Sets.newHashSet(Maps.newHashMap()) produces unchecked warning.

I think it would be simpler to collect these properties into a single, temporary map, and putAll them into the first set outside of forEach. Better yet, use collection methods (removeAll, retainAll) to avoid the loop. Something like this:

Map<String, String> converted = new HashMap<>(clusterEnv); Map<String, String> remaining = new HashMap<>(clusterEnv); remaining.keySet().removeAll(clusterSettingPropertyNames); converted.keySet().retainAll(clusterSettingPropertyNames); setting.addProperties(Setting.SETTING_NAME_CLUSTER_SETTINGS, converted); Sets.intersection(converted.keySet(), SAFE_TO_REMOVE_FROM_CLUSTER_ENV) .forEach(key -> configuration.removeProperty(ConfigHelper.CLUSTER_ENV, key));

With the logic to add properties to Setting extracted to a method in that class:

public void addProperties(String name, Map<String, String> values) { Set<Map<String, String>> set = properties.computeIfAbsent(name, __ -> new HashSet<>(1)); if (set.isEmpty()) { set.add(new HashMap<>(values)); } else { set.iterator().next().putAll(values); } }

…nto AMBARI-24162-branch-feature-AMBARI-14714

asfgit · 2018-06-28T18:01:40Z

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

adoroszlai · 2018-06-28T19:29:24Z

@benyoka something is wrong with the merge, unit test errors increased to 209.

benyoka · 2018-06-28T20:04:17Z

It is mostly caused by BlueprintConfigurationProcessorTest failures.

asfgit · 2018-06-28T21:01:40Z

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

Balazs Bence Sari added 5 commits June 21, 2018 19:46

AMBARI-24162 copy cluster settings from cluster-env + setting fixes (…

d42edb2

…benyoka)

Merge branch 'branch-feature-AMBARI-14714' into AMBARI-24162-branch-f…

7115b6d

…eature-AMBARI-14714

AMBARI-24162 fixint tests WIP (benyoka)

5ad72d5

AMBARI-24162 fix unit tests apache#2 (benyoka)

4bcec0d

AMBARI-24162 write new unit tests (benyoka)

51971b6

benyoka requested review from adoroszlai, mradha25, rnettleton and swapanshridhar June 26, 2018 15:52

rnettleton approved these changes Jun 26, 2018

View reviewed changes

adoroszlai changed the title ~~Ambari 24162 branch feature ambari 14714~~ [AMBARI-24162] Support for cluster-settings (remove cluster-env) Jun 27, 2018

adoroszlai assigned benyoka Jun 27, 2018

adoroszlai added the blueprint label Jun 27, 2018

benyoka requested a review from jayush June 27, 2018 08:55

adoroszlai approved these changes Jun 27, 2018

View reviewed changes

Balazs Bence Sari added 2 commits June 28, 2018 10:37

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

59dd1a1

…nto AMBARI-24162-branch-feature-AMBARI-14714

AMBARI-24162 review commits and unit test fixes (benyoka)

38f5e72

AMBARI-24162 fix blueprint export and blueprint unit tests (benyoka)

8620086

benyoka merged commit 48ddb09 into apache:branch-feature-AMBARI-14714 Jun 29, 2018

                   }
                 }
-                /**

Conversation

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

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

asfgit commented Jun 26, 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!

benyoka Jun 27, 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!

Choose a reason for hiding this comment

Uh oh!

benyoka commented Jun 27, 2018

Uh oh!

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

asfgit commented Jun 28, 2018

Uh oh!

adoroszlai commented Jun 28, 2018

Uh oh!

benyoka commented Jun 28, 2018

Uh oh!

asfgit commented Jun 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benyoka commented Jun 26, 2018 •

edited

Loading

benyoka Jun 27, 2018 •

edited

Loading