Skip to content

Refactor PhaseCacheManagement to apply refreshes in a bulk-safe manner #132660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jbaiera wants to merge 1 commit into from
Closed

Refactor PhaseCacheManagement to apply refreshes in a bulk-safe manner #132660

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/PhaseCacheManagement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,14 @@ public static ProjectMetadata refreshPhaseDefinition(
) {
IndexMetadata idxMeta = projectMetadata.index(index);
ProjectMetadata.Builder projectMetadataBuilder = ProjectMetadata.builder(projectMetadata);
refreshPhaseDefinition(projectMetadataBuilder, idxMeta, updatedPolicy);
projectMetadataBuilder.put(prepareRefreshPhaseDefinition(idxMeta, updatedPolicy));
return projectMetadataBuilder.build();
}

/**
* Rereads the phase JSON for the given index, and updates the provided metadata.
*/
public static void refreshPhaseDefinition(
final ProjectMetadata.Builder projectMetadataBuilder,
public static IndexMetadata.Builder prepareRefreshPhaseDefinition(
final IndexMetadata idxMeta,
final LifecyclePolicyMetadata updatedPolicy
) {
Expand All @@ -83,7 +82,7 @@ public static void refreshPhaseDefinition(
.setPhaseDefinition(Strings.toString(pei, false, false))
.build();

projectMetadataBuilder.put(IndexMetadata.builder(idxMeta).putCustom(ILM_CUSTOM_METADATA_KEY, newExState.asMap()));
return IndexMetadata.builder(idxMeta).putCustom(ILM_CUSTOM_METADATA_KEY, newExState.asMap());
}

/**
Expand Down Expand Up @@ -157,15 +156,19 @@ public static boolean updateIndicesForPolicy(
.filter(meta -> isIndexPhaseDefinitionUpdatable(xContentRegistry, client, meta, newPolicy.getPolicy(), licenseState))
.toList();

final List<String> refreshedIndices = new ArrayList<>(indicesThatCanBeUpdated.size());
final List<IndexMetadata.Builder> refreshedIndices = new ArrayList<>(indicesThatCanBeUpdated.size());
for (IndexMetadata index : indicesThatCanBeUpdated) {
try {
refreshPhaseDefinition(projectMetadataBuilder, index, newPolicy);
refreshedIndices.add(index.getIndex().getName());
var idxBuilder = prepareRefreshPhaseDefinition(index, newPolicy);
refreshedIndices.add(idxBuilder);
} catch (Exception e) {
logger.warn(() -> format("[%s] unable to refresh phase definition for updated policy [%s]", index, newPolicy.getName()), e);
}
Comment on lines 161 to 166
Copy link
Contributor

@samxbr samxbr Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am probably missing something. Since we catch exception with a warning log here and continue with the next index, wouldn't exception down the stack still result in partial update, seems we just delay putting the partial index metadata into the project metadata? I wonder if we should throw an exception instead?

Copy link
Contributor

@samxbr samxbr Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I haven't entirely gone through what's the implication on throwing here.

Copy link
Member Author

@jbaiera jbaiera Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new logic batches up the list of changed index metadata objects and only adds them to the project metadata builder after collecting all of them. Overall, it's a very small refactoring, and practically speaking shouldn't modify any behavior. The bigger picture here is that I'm trying to make changes to the method contract to ensure that the project metadata builder passed is less likely to be modified unnecessarily since it may be carrying changes from another batch of cluster state operations.

Copy link
Contributor

@samxbr samxbr Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very much fine with the small refactoring (without behavior change). I probably misunderstood the PR description This saves from polluting a potentially shared project metadata builder instance with incomplete state updates in the event that an exception is thrown from lower down the call stack thinking it means "all or nothing" refresh on indices. But it just means that we don't update the project metadata until all indices are processed (with or without exception).

}
// Apply new index metadata all at once to avoid half-applications
for (IndexMetadata.Builder refreshedIndex : refreshedIndices) {
projectMetadataBuilder.put(refreshedIndex);
}
logger.debug("refreshed policy [{}] phase definition for [{}] indices", newPolicy.getName(), refreshedIndices.size());
return refreshedIndices.size() > 0;
}
Expand Down