Refactoring TransportSimulateBulkAction to not extend TransportBulkAction #109889
Conversation
masseyke
added
:Data Management/Ingest Node
|
Note: This code was almost entirely taken from the work of @henningandersen at https://github.com/elastic/elasticsearch/compare/main...henningandersen:elasticsearch:improved_simulate_action?expand=1#diff-97ba6f4f3881131b999ea982aa5ab22b31bad9eaf7aa1d14beda660309ae60b7R101 |
elasticsearchmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
The build is currently broken because https://auth.docker.io/ is having problems. |
| ); | ||
| } | ||
|
|
||
| protected abstract boolean shouldStoreFailure(String indexName, Metadata metadata, long epochMillis); |
There was a problem hiding this comment.
Can you add javadocs for this method?
| @@ -781,13 +552,18 @@ public boolean isForceExecution() { | |||
| * @return true if the given index name corresponds to a data stream with a failure store, | |||
| * or if it matches a template that has a data stream failure store enabled. | |||
| */ | |||
There was a problem hiding this comment.
This is probably the javadoc you can copy over
| DocWriteRequest<?> request = bulkRequest.requests.get(i); | ||
| assert request instanceof IndexRequest; // This action is only ever called with IndexRequests | ||
| DocWriteRequest<?> docRequest = bulkRequest.requests.get(i); | ||
| assert docRequest instanceof IndexRequest; // This action is only ever called with IndexRequests |
There was a problem hiding this comment.
Can you change the comment into an assertion message?
|
|
||
| @Override | ||
| protected boolean shouldStoreFailure(String indexName, Metadata metadata, long time) { | ||
| return false; |
There was a problem hiding this comment.
Can you add a comment about why this is always false?
This refactors TransportSimulateBulkAction and TransportBulkAction to inherit from a new common abstract class, rather than having TransportSimulateBulkAction extend TransportBulkAction. No user-observable functionality has changed. The simulate code no longer runs the ShardBulkAction logic (which is not needed for simulate anyway).
The motivation for this change is so that we remove the risk that a simulate request changes any state in the system (index data, cluster state, etc). The tradeoff is that the simulate path now executes less of the logic used by a bulk request (however, most of that logic is irrelevant to a simulation anyway). The core logic of traversing relevant indices and executing pipelines remains shared between simulate and normal bulk.