[AMBARI-23941] Updating serviceLevelParams and clusterLevelParams in server-side metadata holder both when adding and removing a service/property #1366

smolnar82 · 2018-05-24T10:08:19Z

What changes were proposed in this pull request?

When a service has been removed from the cluster its serviceLevelParams stayed in server side's metadata holder but removed from the agent side cache. Next time when someone wanted to re-install the same service there were no metadata change event triggered so that the agent side code failed due to missing serviceLevelParams for that service (the same is true for clusterLevelParams metadata).

How was this patch tested?

Added unit test code to cover these cases; latest JUnit test results in ambari-server:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 02:12 h
[INFO] Finished at: 2018-05-24T03:33:29+02:00
[INFO] Final Memory: 107M/1592M
[INFO] ------------------------------------------------------------------------

In addition to uni testing the following integration test steps were executed:

installed Ambari 2.7.0.0-562
replaced ambari-server.jar with mine after modifying and building the code
deployed a cluster: HDFS via BP and then ZK and Kafka on UI
removed ZK and Kafka
added ZK and Kafka again without any issue (when reproducing the issue I as not able to re-install any of them)

…erver-side metadata holder both when adding and removing a service/property

asfgit · 2018-05-24T10:56:03Z

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

adoroszlai

Looks good except a typo.

adoroszlai · 2018-05-24T11:29:24Z

ambari-server/src/main/java/org/apache/ambari/server/agent/stomp/dto/MetadataCluster.java

@@ -87,7 +87,16 @@ public boolean updateServiceLevelParams(SortedMap<String, MetadataServiceInfo> u
        break;
      }
    }
+
+    for (String key : serviceLevelParams.keySet()) {
+      if (!update.containsKey(key) || !update.get(key).equals(update.get(key))) {


There is a typo in this line: update.get(key) is compared to itself (update.get(key)) instead of serviceLevelParams.get(key).

Further, I think this comparison can be removed: values for keys present in both maps are already compared in the first loop.

Hm...this is weird..I fixed this issue long before I created the PR (it also broker unit tests). Let me check my stash.

adoroszlai · 2018-05-24T11:34:32Z

ambari-server/src/main/java/org/apache/ambari/server/agent/stomp/dto/MetadataCluster.java

+        break;
+      }
+    }
+


This method is almost identical to updateServiceLevelParams. (The duplication is not introduced in this change, but is highlighted by the fact that the same problem needs to be fixed in two places.) Can you please extract a method that takes 2 maps and updates one of them from the other? Then please delegate to that method from these two. Please make sure to keep the version that uses null-safe comparison.

Good idea; I'll extract it to a util method (maybe an existing/new class; will check this out)

rlevas · 2018-05-24T12:13:00Z

@jonathan-hurley , this seems like it may be familiar to you. If so, then maybe there is a common solution to your issues.

jonathan-hurley · 2018-05-24T12:18:50Z

@rlevas No, my problem is separate from this one. This is simply keeping the agent and server caches in sync. The issue I'm having is that when adding or removing services/components, a massive event needs to be fired fill with data - and it doesn't happen automatically.

rlevas · 2018-05-24T12:20:38Z

@jonathan-hurley , Thanks for commenting so quickly.

zeroflag · 2018-05-24T12:44:58Z

Looks good, but can you check if we need to guarantee thread safety somehow in the update method or do we always call this on a single thread?

smolnar82 · 2018-05-24T16:29:03Z

@zeroflag I'll take care of proper locking being applied (even if it was modified on 1 thread it does not harm).

…dating service/cluster level parameters

smolnar82 · 2018-05-24T19:12:14Z

@adoroszlai @zeroflag @rlevas @mpapirkovskyy please review again; thx!

asfgit · 2018-05-24T19:58:05Z

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

adoroszlai · 2018-05-24T20:33:34Z

@smolnar82 Thanks for the changes, they look good with respect to what was requested.

However, on second thought, I have some concern about how this entire change would work with "partial" updates. For example, the following method passes service-level params only for a single service. With your change, wouldn't this kind of update get rid of all other existing services from the metadata?

ambari/ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java

Lines 5652 to 5655 in e9374c9

    
           MetadataCluster metadataCluster = new MetadataCluster(null, 
        
               getMetadataServiceLevelParams(cl.getService(serviceName)), 
        
               new TreeMap<>(), 
        
               null);

Similarly, there are cases where empty map in update means "no changes" in that part of the metadata:

ambari/ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java

Lines 5620 to 5623 in e9374c9

    
           MetadataCluster metadataCluster = new MetadataCluster(null, 
        
               new TreeMap<>(), 
        
               getMetadataClusterLevelConfigsParams(cl, stackId), 
        
               null);

ambari/ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java

Lines 5634 to 5637 in e9374c9

    
           MetadataCluster metadataCluster = new MetadataCluster(null, 
        
               getMetadataServiceLevelParams(cl), 
        
               new TreeMap<>(), 
        
               null);

Wouldn't these updates clear service- or cluster-level params?

smolnar82 · 2018-05-24T23:27:05Z

@adoroszlai
Thanks for your comment. Let me think it over again and give a meaningful answer to make your concerns go away or change the code if needed.

In the meantime I'd really appreciate if @mpapirkovskyy would review this change and let me know if this is the approach what we should follow. Thanks in advance!

mpapirkovskyy

I also tend to think that agent change to not delete params may be better/easier for now, so it will be in sync with server.

I don't think some logic in agent scripts can rely on presence/absence of param rather than its value.

@aonishuk any thoughts?

mpapirkovskyy · 2018-05-25T10:58:32Z

ambari-server/src/main/java/org/apache/ambari/server/agent/stomp/dto/MetadataCluster.java

-  private Set<String> statusCommandsToRun = new HashSet<>();
-  private SortedMap<String, MetadataServiceInfo> serviceLevelParams = new TreeMap<>();
-  private SortedMap<String, String> clusterLevelParams = new TreeMap<>();
+  private final static Lock LOCK = new ReentrantLock();


Any reason for static lock?
There are multiple instances of this object and all of them are going use single lock instance.
It looks like theres better place for lock, and again not for static one:
org.apache.ambari.server.agent.stomp.MetadataHolder#handleUpdate

Another possibility is to use ConcurrentSkipListMap

It makes sense to have the lock a simple one (i.e. not static). However I'd keep it here to make sure we lock only the update piece and do not spend more time on locking where it is not necessary.

mpapirkovskyy · 2018-05-25T11:01:31Z

ambari-server/src/main/java/org/apache/ambari/server/agent/stomp/dto/MetadataCluster.java

+      final boolean changed = !Objects.equals(mapToBeUpdated, updatedMap);
+
+      if (changed) {
+        mapToBeUpdated.clear();


I agree with @adoroszlai that this is going to break partial metadata updates.
So we should either always send full metadata which will require additional modifications.
Or change logic here.

I'll fix it soon

….e. service or cluster level params should not be touched) and for the case when service level params are updated for a single service only (in case of cluter level params we always use full metadata)

smolnar82 · 2018-05-29T08:05:14Z

@adoroszlai @mpapirkovskyy @zeroflag @rlevas please review my changes; thanks in advance!

I re-executed the integration test I described above and re-run the unit tests in ambari-server:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 44:11 min
[INFO] Finished at: 2018-05-29T08:01:32+02:00
[INFO] Final Memory: 106M/1505M
[INFO] ------------------------------------------------------------------------

asfgit · 2018-05-29T08:47:43Z

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

smolnar82 · 2018-05-30T07:10:53Z

@adoroszlai @mpapirkovskyy @zeroflag @rlevas please review my changes; thanks in advance!

adoroszlai

LGTM, except minor nit about locking.

adoroszlai · 2018-05-30T07:24:36Z

ambari-server/src/main/java/org/apache/ambari/server/agent/stomp/dto/MetadataCluster.java

+  public boolean updateServiceLevelParams(SortedMap<String, MetadataServiceInfo> update, boolean fullMetadataInUpdatedMap) {
+    if (update != null) {
+      if (this.serviceLevelParams == null) {
+        this.serviceLevelParams = new TreeMap<>();


I think these map creations should also be guarded by lock.

You are right (this was a last minute movement at certainly not thought over); it's fixed now.

asfgit · 2018-05-30T09:48:04Z

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

smolnar82 · 2018-05-30T10:04:20Z

retest this please

asfgit · 2018-05-30T10:41:32Z

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

aonishuk

@mpapirkovskyy the solution to do this on agent sounds hacky to me, should be done by server by all logic.

However it's possible, but I think it would be better the server and agent data are in synch to avoid other issues caused by this hack.

smolnar82 · 2018-05-30T13:48:38Z

retest this please

adoroszlai · 2018-05-30T13:51:47Z

@smolnar82 I think the fix for broken unit test needs to be merged from origin/trunk and pushed here to successfully "retest".

asfgit · 2018-05-30T14:38:59Z

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

smolnar82 · 2018-05-30T15:05:02Z

@adoroszlai if that was the case I should not have even seen the error since I did not merge anything from origin/trunk during my patches to avoid force pushing

AMBARI-23941. Updating serviceLevelParams and clusterLevelParams in s…

90eb9c4

…erver-side metadata holder both when adding and removing a service/property

smolnar82 requested review from mpapirkovskyy, zeroflag, a user, adoroszlai, rlevas and aonishuk May 24, 2018 10:08

smolnar82 self-assigned this May 24, 2018

ghost approved these changes May 24, 2018

View reviewed changes

adoroszlai suggested changes May 24, 2018

View reviewed changes

rlevas requested a review from jonathan-hurley May 24, 2018 12:12

AMBARI-23941. Eliminated code duplication and applied locking when up…

cae8c93

…dating service/cluster level parameters

mpapirkovskyy suggested changes May 25, 2018

View reviewed changes

AMBARI-23941. Preparing the code for partial configuration updates (i…

3a8b7a2

….e. service or cluster level params should not be touched) and for the case when service level params are updated for a single service only (in case of cluter level params we always use full metadata)

adoroszlai reviewed May 30, 2018

View reviewed changes

AMBARI-23941. Locking service/cluster level metadata maps properly

6951123

adoroszlai approved these changes May 30, 2018

View reviewed changes

aonishuk approved these changes May 30, 2018

View reviewed changes

smolnar82 merged commit 02da908 into apache:trunk May 30, 2018

smolnar82 deleted the AMBARI-23941 branch May 30, 2018 15:05

[AMBARI-23941] Updating serviceLevelParams and clusterLevelParams in server-side metadata holder both when adding and removing a service/property #1366

[AMBARI-23941] Updating serviceLevelParams and clusterLevelParams in server-side metadata holder both when adding and removing a service/property #1366

Conversation

smolnar82 commented May 24, 2018 • edited

What changes were proposed in this pull request?

How was this patch tested?

asfgit commented May 24, 2018

adoroszlai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlevas commented May 24, 2018

jonathan-hurley commented May 24, 2018

rlevas commented May 24, 2018

zeroflag commented May 24, 2018

smolnar82 commented May 24, 2018

smolnar82 commented May 24, 2018 • edited

asfgit commented May 24, 2018

adoroszlai commented May 24, 2018

smolnar82 commented May 24, 2018 • edited

mpapirkovskyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smolnar82 commented May 29, 2018

asfgit commented May 29, 2018

smolnar82 commented May 30, 2018

adoroszlai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfgit commented May 30, 2018

smolnar82 commented May 30, 2018

asfgit commented May 30, 2018

aonishuk left a comment

Choose a reason for hiding this comment

smolnar82 commented May 30, 2018

adoroszlai commented May 30, 2018

asfgit commented May 30, 2018

smolnar82 commented May 30, 2018

smolnar82 commented May 24, 2018 •

edited

smolnar82 commented May 24, 2018 •

edited

smolnar82 commented May 24, 2018 •

edited