Use an index to store enrich policies #47475
Conversation
This commit changes the enrich store's backing storage. It was previously stored in cluster state, and now it is stored in an index. A side effect of this is that listeners had to be added in the enrich stores API. The actions no longer need to be master actions, so they have been changed too. The named writable that was used in cluster state is also deleted in this commit.
hub-cap
added
>non-issue
:Data Management/Ingest Node
|
Pinging @elastic/es-core-features (:Core/Features/Ingest) |
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
This looks good! I left of couple of comments.
|
|
||
| if (enrichIndex == null) { | ||
| // create the index | ||
| client.admin().indices().prepareCreate(ENRICH_INDEX) |
There was a problem hiding this comment.
Like we discussed in chat, we should either use index template or keep using the create index api call with the right settings and mappings.
| import java.util.stream.Collectors; | ||
|
|
||
| public class TransportGetEnrichPolicyAction extends TransportMasterNodeReadAction<GetEnrichPolicyAction.Request, | ||
| public class TransportGetEnrichPolicyAction extends HandledTransportAction<GetEnrichPolicyAction.Request, |
There was a problem hiding this comment.
Can the get policy api remain a master node action? I think we are going to add additional things to this API for the UI. Like status and that information can only be read from elected master node. (this is where the policy executor lives)
| } | ||
|
|
||
| @Override | ||
| public List<NamedWriteableRegistry.Entry> getNamedWriteables() { |
| public static void putPolicy(String name, EnrichPolicy policy, ClusterService clusterService, Consumer<Exception> handler) { | ||
| assert clusterService.localNode().isMasterNode(); | ||
|
|
||
| public static void putPolicy(String name, EnrichPolicy policy, ClusterService clusterService, Client client, |
There was a problem hiding this comment.
Maybe replace the ClusterService parameter with ClusterState here and in other methods, since we only seem to invoke ClusterService#state() method now.
| @Override | ||
| public void onFailure(Exception e) { | ||
| logger.error("Failed to get indices during enrich index maintenance task", e); | ||
| EnrichStore.getPolicies(clusterService.state(), client, ActionListener.wrap( |
There was a problem hiding this comment.
Perhaps the get policies call should be done inside:
final EnrichPolicyLocks.EnrichPolicyExecutionState executionState = enrichPolicyLocks.captureExecutionState();
if (executionState.isAnyPolicyInFlight() == false) {
...
}
|
After discussing the use of the template registry for setting up the index for enrich, we came to the conclusion that we should not use an index, and rely on cluster state which is the default. The policies are small, we get a lot of things for free such as getting the policies when we query cluster state doing diagnostics, and we dont have to worry about the index going away during a migration or some failure that can occur with indexes that will generally speaking not occur in cluster state. There is a chance we can intro a bug into the cluster state, and while that is annoying, it can be fixed. These reasons led us to decide to stop this work and let it continue to be in cluster state. |
3 participants
This commit changes the enrich store's backing storage. It was
previously stored in cluster state, and now it is stored in an index. A
side effect of this is that listeners had to be added in the enrich
stores API. The actions no longer need to be master actions, so they
have been changed too. The named writable that was used in cluster state
is also deleted in this commit.