Skip to content

Commit e4c0fa9

Browse files
nielsbaumannielsbauman
authored
Fix some lazy rollover code (#124153) (#124309)
The main goal of the PR is to fix the "rollover failed" log in the `IndexTemplateRegistry`. We were logging this on every upgrade because `rolloverResponse.isRolledOver()` is always `false` when we do a lazy rollover request - which is the default for index template updates. While I was looking at this, I noticed we don't actually return early when we process a lazy rollover _dry run_. The end result happened to be ok (i.e. side-effects and response), but I figured I might as well fix the behavior to return early. Finally, we add an `INFO` log in the lazy rollover action to aid the debugging process. (cherry picked from commit 9544204) # Conflicts: # server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java
1 parent 236b458 commit e4c0fa9

File tree

4 files changed

+103
-54
lines changed

4 files changed

+103
-54
lines changed

‎server/src/main/java/org/elasticsearch/action/admin/indices/rollover/LazyRolloverAction.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,11 @@ public ClusterState executeTask(
260260

261261
final var rolloverIndexName = rolloverResult.rolloverIndexName();
262262
final var sourceIndexName = rolloverResult.sourceIndexName();
263+
logger.info(
264+
"rolling over data stream [{}] to index [{}] because it was marked for lazy rollover",
265+
dataStream.getName(),
266+
rolloverIndexName
267+
);
263268

264269
final var waitForActiveShardsTimeout = rolloverRequest.masterNodeTimeout().millis() < 0
265270
? null

‎server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java‎

Lines changed: 62 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -212,44 +212,17 @@ protected void masterOperation(
212212
final String trialRolloverIndexName = trialRolloverNames.rolloverName();
213213
MetadataCreateIndexService.validateIndexName(trialRolloverIndexName, metadata, clusterState.routingTable());
214214

215-
boolean isDataStream = metadata.dataStreams().containsKey(resolvedRolloverTarget.resource());
216215
if (rolloverRequest.isLazy()) {
217-
if (isDataStream == false || rolloverRequest.getConditions().hasConditions()) {
218-
String message;
219-
if (isDataStream) {
220-
message = "Lazy rollover can be used only without any conditions."
221-
+ " Please remove the conditions from the request body or the query parameter 'lazy'.";
222-
} else if (rolloverRequest.getConditions().hasConditions() == false) {
223-
message = "Lazy rollover can be applied only on a data stream." + " Please remove the query parameter 'lazy'.";
224-
} else {
225-
message = "Lazy rollover can be applied only on a data stream with no conditions."
226-
+ " Please remove the query parameter 'lazy'.";
227-
}
228-
listener.onFailure(new IllegalArgumentException(message));
229-
return;
230-
}
231-
if (rolloverRequest.isDryRun() == false) {
232-
metadataDataStreamsService.setRolloverOnWrite(
233-
resolvedRolloverTarget.resource(),
234-
true,
235-
targetFailureStore,
236-
rolloverRequest.ackTimeout(),
237-
rolloverRequest.masterNodeTimeout(),
238-
listener.map(
239-
response -> new RolloverResponse(
240-
trialSourceIndexName,
241-
trialRolloverIndexName,
242-
Map.of(),
243-
false,
244-
false,
245-
response.isAcknowledged(),
246-
false,
247-
response.isAcknowledged()
248-
)
249-
)
250-
);
251-
return;
252-
}
216+
markForLazyRollover(
217+
rolloverRequest,
218+
listener,
219+
metadata,
220+
resolvedRolloverTarget,
221+
targetFailureStore,
222+
trialSourceIndexName,
223+
trialRolloverIndexName
224+
);
225+
return;
253226
}
254227

255228
final IndexAbstraction rolloverTargetAbstraction = clusterState.metadata()
@@ -345,7 +318,7 @@ protected void masterOperation(
345318
false,
346319
false,
347320
false,
348-
rolloverRequest.isLazy()
321+
false
349322
);
350323

351324
// If this is a dry run, return with the results without invoking a cluster state update
@@ -373,6 +346,57 @@ protected void masterOperation(
373346
);
374347
}
375348

349+
private void markForLazyRollover(
350+
RolloverRequest rolloverRequest,
351+
ActionListener<RolloverResponse> listener,
352+
Metadata metadata,
353+
ResolvedExpression resolvedRolloverTarget,
354+
boolean targetFailureStore,
355+
String trialSourceIndexName,
356+
String trialRolloverIndexName
357+
) {
358+
boolean isDataStream = metadata.dataStreams().containsKey(resolvedRolloverTarget.resource());
359+
if (isDataStream == false || rolloverRequest.getConditions().hasConditions()) {
360+
String message;
361+
if (isDataStream) {
362+
message = "Lazy rollover can be used only without any conditions."
363+
+ " Please remove the conditions from the request body or the query parameter 'lazy'.";
364+
} else if (rolloverRequest.getConditions().hasConditions() == false) {
365+
message = "Lazy rollover can be applied only on a data stream. Please remove the query parameter 'lazy'.";
366+
} else {
367+
message = "Lazy rollover can be applied only on a data stream with no conditions."
368+
+ " Please remove the query parameter 'lazy'.";
369+
}
370+
listener.onFailure(new IllegalArgumentException(message));
371+
return;
372+
}
373+
if (rolloverRequest.isDryRun()) {
374+
listener.onResponse(
375+
new RolloverResponse(trialSourceIndexName, trialRolloverIndexName, Map.of(), true, false, false, false, true)
376+
);
377+
return;
378+
}
379+
metadataDataStreamsService.setRolloverOnWrite(
380+
resolvedRolloverTarget.resource(),
381+
true,
382+
targetFailureStore,
383+
rolloverRequest.ackTimeout(),
384+
rolloverRequest.masterNodeTimeout(),
385+
listener.map(
386+
response -> new RolloverResponse(
387+
trialSourceIndexName,
388+
trialRolloverIndexName,
389+
Map.of(),
390+
false,
391+
false,
392+
response.isAcknowledged(),
393+
false,
394+
true
395+
)
396+
)
397+
);
398+
}
399+
376400
private void initializeFailureStore(
377401
RolloverRequest rolloverRequest,
378402
ActionListener<RolloverResponse> listener,

‎server/src/test/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverActionTests.java‎

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -466,19 +466,39 @@ public void testLazyRollover() throws Exception {
466466
mockMetadataDataStreamService,
467467
dataStreamAutoShardingService
468468
);
469-
final PlainActionFuture<RolloverResponse> future = new PlainActionFuture<>();
470-
RolloverRequest rolloverRequest = new RolloverRequest("logs-ds", null);
471-
rolloverRequest.lazy(true);
472-
transportRolloverAction.masterOperation(mock(CancellableTask.class), rolloverRequest, stateBefore, future);
473-
RolloverResponse rolloverResponse = future.actionGet();
474-
assertThat(rolloverResponse.getOldIndex(), equalTo(".ds-logs-ds-000001"));
475-
assertThat(rolloverResponse.getNewIndex(), Matchers.startsWith(".ds-logs-ds-"));
476-
assertThat(rolloverResponse.getNewIndex(), Matchers.endsWith("-000002"));
477-
assertThat(rolloverResponse.isLazy(), equalTo(true));
478-
assertThat(rolloverResponse.isDryRun(), equalTo(false));
479-
assertThat(rolloverResponse.isRolledOver(), equalTo(false));
480-
assertThat(rolloverResponse.getConditionStatus().size(), equalTo(0));
481-
assertThat(rolloverResponse.isAcknowledged(), is(true));
469+
{
470+
// Regular lazy rollover
471+
final PlainActionFuture<RolloverResponse> future = new PlainActionFuture<>();
472+
RolloverRequest rolloverRequest = new RolloverRequest("logs-ds", null);
473+
rolloverRequest.lazy(true);
474+
transportRolloverAction.masterOperation(mock(CancellableTask.class), rolloverRequest, stateBefore, future);
475+
RolloverResponse rolloverResponse = future.actionGet();
476+
assertThat(rolloverResponse.getOldIndex(), equalTo(".ds-logs-ds-000001"));
477+
assertThat(rolloverResponse.getNewIndex(), Matchers.startsWith(".ds-logs-ds-"));
478+
assertThat(rolloverResponse.getNewIndex(), Matchers.endsWith("-000002"));
479+
assertThat(rolloverResponse.isLazy(), equalTo(true));
480+
assertThat(rolloverResponse.isDryRun(), equalTo(false));
481+
assertThat(rolloverResponse.isRolledOver(), equalTo(false));
482+
assertThat(rolloverResponse.getConditionStatus().size(), equalTo(0));
483+
assertThat(rolloverResponse.isAcknowledged(), is(true));
484+
}
485+
{
486+
// Dry-run lazy rollover
487+
final PlainActionFuture<RolloverResponse> future = new PlainActionFuture<>();
488+
RolloverRequest rolloverRequest = new RolloverRequest("logs-ds", null);
489+
rolloverRequest.lazy(true);
490+
rolloverRequest.dryRun(true);
491+
transportRolloverAction.masterOperation(mock(CancellableTask.class), rolloverRequest, stateBefore, future);
492+
RolloverResponse rolloverResponse = future.actionGet();
493+
assertThat(rolloverResponse.getOldIndex(), equalTo(".ds-logs-ds-000001"));
494+
assertThat(rolloverResponse.getNewIndex(), Matchers.startsWith(".ds-logs-ds-"));
495+
assertThat(rolloverResponse.getNewIndex(), Matchers.endsWith("-000002"));
496+
assertThat(rolloverResponse.isLazy(), equalTo(true));
497+
assertThat(rolloverResponse.isDryRun(), equalTo(true));
498+
assertThat(rolloverResponse.isRolledOver(), equalTo(false));
499+
assertThat(rolloverResponse.getConditionStatus().size(), equalTo(0));
500+
assertThat(rolloverResponse.isAcknowledged(), is(false));
501+
}
482502
}
483503

484504
public void testLazyRolloverFails() throws Exception {

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/IndexTemplateRegistry.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.elasticsearch.cluster.metadata.Template;
3434
import org.elasticsearch.cluster.node.DiscoveryNode;
3535
import org.elasticsearch.cluster.service.ClusterService;
36+
import org.elasticsearch.common.Strings;
3637
import org.elasticsearch.common.regex.Regex;
3738
import org.elasticsearch.common.settings.Settings;
3839
import org.elasticsearch.core.Nullable;
@@ -840,9 +841,8 @@ public void onFailure(Exception e) {
840841

841842
void onRolloversBulkResponse(Collection<RolloverResponse> rolloverResponses) {
842843
for (RolloverResponse rolloverResponse : rolloverResponses) {
843-
if (rolloverResponse.isRolledOver() == false) {
844-
logger.warn("rollover of the [{}] index [{}] failed", getOrigin(), rolloverResponse.getOldIndex());
845-
}
844+
assert rolloverResponse.isLazy() && rolloverResponse.isRolledOver() == false
845+
: Strings.format("Expected rollover of the [%s] index [%s] to be lazy", getOrigin(), rolloverResponse.getOldIndex());
846846
}
847847
}
848848

0 commit comments

Comments
 (0)