Add support for merging custom meta data in tribe node by areek · Pull Request #21552 · elastic/elasticsearch

areek · 2016-11-14T19:28:34Z

Currently, when any underlying cluster has custom metadata
(via plugin), tribe node does not store custom meta data in its
cluster state. This is because the tribe node has no idea how to
select the appropriate custom metadata from one or many custom
metadata (corresponding to the number of underlying clusters).

This change adds an interface that custom metadata implementations
can extend to add support for merging mulitple custom metadata of
the same type for storing in the tribe state.

Relates to #20544
Supersedes #20791
Closes #9372

areek · 2016-11-14T19:29:35Z

@bleskes can you take a look?

javanna · 2016-11-14T20:59:29Z

Seems like this PR fixes #9372 ?

bleskes · 2016-11-15T12:28:27Z

FYI @areek is working on allowing custom type instances to be removed as well (which is currently not part of the PR)

areek · 2016-11-15T20:07:43Z

@javanna this does implement a solution for #9372, it adds the merge functionality only for MetaData.Customs.
@bleskes I updated the PR according to our discussion offline, now we support removing custom types as well, can you take a look?

bleskes

This looks very good. I left some comments that I think will simplify the code.

bleskes · 2016-11-17T07:03:15Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

nit: can we document that the neither original customs should be changed by this method?

added it to the docs

bleskes · 2016-11-17T07:09:35Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

this method became useless I think? the whole try catch can be removed as it's handled by the caller in the ClusterService. I think we should break applyUpdate into several methods each updating a different parts. i.e., updateNodes, updateIndices and now the now updateCustoms

Thanks for the suggestion. I decomposed applyUpdate to two methods updateNodes and updateIndicesAndMetaData instead. Currently, MetaData is updated when we update indices and when we update the customs, so it made sense to updateCustoms when we are done updating MetaData for updating indices. I think it would be messy to try to update MetaData in two separate methods from the top-level execute

bleskes · 2016-11-17T07:13:31Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

Why do you we need the custom of the tribe node?

Apologies for the misleading comment (updated it now). This special cases getting the custom for the tribeClient that triggered the current cluster state update. Because we have the latest cluster state from the clusterChangedEvent for this tribeClient, we use that instead of looking up the state from the tribeClient's cluster service.
I think this is the right thing to do here, though I have tested it out without this special case and it still works. I am worried if the tribeClient's state from the cluster service (which initiated the cluster state update) is outdated w.r.t its latest changes. WDYT @bleskes ?

bleskes · 2016-11-17T07:14:32Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

nit: can we have this ugliness in a getClusterService(Node node) method? we use it in doStart as well.

bleskes · 2016-11-17T07:16:59Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

can we make this a proper getCurrentCustomsFromNodes method and test it etc?

we only get a list of MergableCustomMetaDatas from this for all the tribeClient nodes and if we do the special casing as mentioned here, extracting it to a static function and testing it would be difficult. I think we are better off beefing up the integration tests TribeIT to deal with multiple customs instead?

I added tribe integration test for merging multiple custom metadata types

bleskes · 2016-11-17T07:19:20Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

do we really need this extra protection? I mean the type was already in our changed type list?

no we don't, I removed the extra protection

bleskes · 2016-11-17T07:21:05Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

I don't think you need an if else structure here? you can set the first and iterate over the rest? This can be simplified

I simplified it as suggested

Currently, when any underlying cluster has custom metadata (via plugin), tribe node does not store custom meta data in its cluster state. This is because the tribe node has no idea how to select the appropriate custom metadata from one or many custom metadata (corresponding to the number of underlying clusters). This change adds an interface that custom metadata implementations can extend to add support for merging mulitple custom metadata of the same type for storing in the tribe state. Relates to elastic#20544 Supersedes elastic#20791

areek · 2016-11-17T21:33:29Z

Thanks for the feedback @bleskes! I addressed your comments, left a question and added more tests. could you take another look?

bleskes

Thx @areek . I left some more comments. I would be great to have a test that tests batching incoming cluster states as well.

bleskes · 2016-11-18T07:36:44Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

accumulator is not currentstate?

thinking about it more - do we even need this accumulator?

I cleaned up the code and removed the redundant accumulator

bleskes · 2016-11-18T07:50:55Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

Answering here - I would prefer keeping the logic the same for all nodes i.e., any custom change is processed based on the latest cluster state of those nodes. No need to special case the current node as I don't see what it buys us but complexity? without it it can be a simple stream

okay, I am assuming tribe node receives the cluster changed event only after the change has been applied to the underlying cluster. I removed the special casing for this and converted into a simple stream

yes it is - see here vs here.

Might be good to add an assertion the tribe listener to see that the cluster state is already exposed when it's called - this will prevent future trouble :)

bleskes · 2016-11-18T07:51:29Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

this can be out of if now.

bleskes · 2016-11-18T07:55:25Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

we can make this a function to List<MergableCustomMetaData> - we alreay check with instance of in the implementation of it.

if we do so, I think this becomes a simple stream reduce

changed it to accept a function returning List<MergableCustomMetaData and doing so did simplify the mergeChangedCustomMetaData function to a stream reduce

bleskes · 2016-11-18T07:59:14Z

core/src/test/java/org/elasticsearch/tribe/TribeServiceTests.java

I think we need some testing with removed customs? i.e., nulls?

the mergeChangedCustomMetaData never gets null values for removed customs, instead they are not supplied by the customMetaDataByTribeNode function at all when a mergable custom metadata has been removed. I added tests for removing customs in TribeIT (testMergingRemovedCustomMetaData and testMergingMultipleCustomMetaData)

bleskes

LGTM. I like the last iteration a lot. Did you see my question about testing bulk processing of incoming states? Maybe I missed it.

bleskes · 2016-11-19T06:36:39Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

-                        }
-                        return tribeCustomMetaDataList;
-                    }
+                    customMetaDataType -> tribeClientNodes.stream()


bleskes · 2016-11-19T06:43:21Z

core/src/main/java/org/elasticsearch/tribe/TribeService.java

+                MetaData.Custom mergedCustomMetaData = mergedCustomMetaDataMap.get(changedCustomMetaDataType);
+                if (mergedCustomMetaData == null) {
+                    // we ignore merging custom md which doesn't implement MergableCustomMetaData interface
+                    if (currentState.metaData().custom(changedCustomMetaDataType) instanceof MergableCustomMetaData) {


I'm wondering if we really need this check - when is it relevant?

when a non-mergable custom md changes, it is in changedCustomMetaDataTypeSet but is ignored by mergeChangedCustomMetaData (returns a null for the custom type). So we can delete non-mergable custom md from the tribe cluster, while trying to merge mergable custom md. This check prevents deleting these non-mergable custom md.

areek · 2016-11-19T19:06:33Z

@bleskes I am working on writing some simple unit tests for TribeNodeClusterStateTaskExecutor (by making it to a static class), I plan on testing the bulk processing of incoming states there

areek · 2016-11-21T17:00:47Z

I am going to merge this to master and backport it to 5.x branch now. Will add the unit tests (including tsting bulk processing of incoming states) for TribeNodeClusterStateTaskExecutor in a subsequent PR.

* Add support for merging custom meta data in tribe node Currently, when any underlying cluster has custom metadata (via plugin), tribe node does not store custom meta data in its cluster state. This is because the tribe node has no idea how to select the appropriate custom metadata from one or many custom metadata (corresponding to the number of underlying clusters). This change adds an interface that custom metadata implementations can extend to add support for merging mulitple custom metadata of the same type for storing in the tribe state. Relates to #20544 Supersedes #20791 * Simplify updating tribe state * Add tests for merging multiple custom metadata types in tribe node * cleanup merging custom md logic in tribe service

areek · 2016-11-21T18:05:05Z

Thanks for the reviews, @bleskes! merged to master and backported to 5.x

* master: (42 commits) Add support for merging custom meta data in tribe node (elastic#21552) [DOCS] Show EC2's auto attribute (elastic#21474) Add information about the removal of store throttling to the migration guide. Add a recommendation against large documents to the docs. (elastic#21652) Add indices options tests to search api REST tests (elastic#21701) Fixing indentation in geospatial querying example. (elastic#21682) Fix typo in filters aggregation docs (elastic#21690) Add BWC layer for Exceptions (elastic#21694) Add checkstyle rule to forbid empty javadoc comments (elastic#20881) Docs: Added offline install link for discovery-file plugin remove pointless catch exception in TransportSearchAction (elastic#21689) Rename ClusterState#lookupPrototypeSafe to `lookupPrototype` and remove previous "unsafe" unused variant (elastic#21686) Use a buffer to do character to byte conversion in StreamOutput#writeString (elastic#21680) Fix integer overflows when dealing with templates. (elastic#21628) Fix highlighting on a stored keyword field (elastic#21645) Set execute permissions for native plugin programs (elastic#21657) adjust visibility of DiscoveryNodes.Delta constructor Remove unused DiscoveryNodes.Delta constructor Remove unused DiscoveryNode#removeDeadMembers public method Remove minNodeVersion and corresponding public `getSmallestVersion` getter method from DiscoveryNodes ...

This commit makes TribeNodeClusterStateTaskExecutor a static class for unit testing. TribeNodeClusterStateTaskExecutor is responsible for applying cluster state updates to the tribe node state whenever an underlying cluster state is updated (i.e. adding/removing indices, updating nodes and merging custom metadata). The unit tests ensure the tribe state is properly updating when underlying cluster state tasks are seen by the tribe node in batches or as a single update. relates elastic#21552

areek added :Tribe Node >enhancement v5.1.1 v6.0.0-alpha1 labels Nov 14, 2016

areek mentioned this pull request Nov 14, 2016

Add support for storing custom metadata in tribe node #20791

Closed

areek force-pushed the enhancement/tribe-merge-custom-md branch from 874bcd4 to a308ffa Compare November 14, 2016 20:02

martijnvg mentioned this pull request Nov 14, 2016

Better extension point for custom cluster state parts #21243

Closed

areek force-pushed the enhancement/tribe-merge-custom-md branch 2 times, most recently from e6c20b1 to 2e41eb2 Compare November 15, 2016 20:04

areek force-pushed the enhancement/tribe-merge-custom-md branch from 2e41eb2 to a3a74c8 Compare November 15, 2016 21:13

bleskes suggested changes Nov 17, 2016

View reviewed changes

areek force-pushed the enhancement/tribe-merge-custom-md branch 2 times, most recently from 6becea3 to b216ac5 Compare November 17, 2016 20:10

Simplify updating tribe state

cec9960

areek force-pushed the enhancement/tribe-merge-custom-md branch from b216ac5 to cec9960 Compare November 17, 2016 20:15

Add tests for merging multiple custom metadata types in tribe node

16d6500

areek force-pushed the enhancement/tribe-merge-custom-md branch from 83ac215 to 16d6500 Compare November 18, 2016 07:52

bleskes suggested changes Nov 18, 2016

View reviewed changes

areek force-pushed the enhancement/tribe-merge-custom-md branch from 7caf1a3 to 7a54c67 Compare November 18, 2016 21:30

cleanup merging custom md logic in tribe service

ae4c137

areek force-pushed the enhancement/tribe-merge-custom-md branch from 7a54c67 to ae4c137 Compare November 18, 2016 21:35

bleskes approved these changes Nov 19, 2016

View reviewed changes

areek merged commit 0ccf8a7 into elastic:master Nov 21, 2016

areek mentioned this pull request Nov 29, 2016

Add unit test for TribeNodeClusterStateTaskExecutor #21856

Closed

Conversation

areek commented Nov 14, 2016 • edited by clintongormley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

areek commented Nov 14, 2016

Uh oh!

javanna commented Nov 14, 2016

Uh oh!

bleskes commented Nov 15, 2016

Uh oh!

areek commented Nov 15, 2016

Uh oh!

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

areek commented Nov 17, 2016

Uh oh!

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

bleskes 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

areek commented Nov 14, 2016 •

edited by clintongormley

Loading

areek commented Nov 19, 2016 •

edited

Loading