Reduce WaitForNoFollowersStep requests indices shard stats #94510

luyuncheng · 2023-03-15T04:36:47Z

When data is time series indices, it may concurrent create many index and delete index using ILM

i see DeleteAction, ShrinkAction need to check ccr status in WaitForNoFollowersStep as following code shows:

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteAction.java

Lines 81 to 96 in 3962556

     public List<Step> toSteps(Client client, String phase, Step.StepKey nextStepKey) {  
   Step.StepKey waitForNoFollowerStepKey = new Step.StepKey(phase, NAME, WaitForNoFollowersStep.NAME);  
   Step.StepKey deleteStepKey = new Step.StepKey(phase, NAME, DeleteStep.NAME);  
   Step.StepKey cleanSnapshotKey = new Step.StepKey(phase, NAME, CleanupSnapshotStep.NAME);  
    
   if (deleteSearchableSnapshot) {  
   WaitForNoFollowersStep waitForNoFollowersStep = new WaitForNoFollowersStep(waitForNoFollowerStepKey, cleanSnapshotKey, client);  
   CleanupSnapshotStep cleanupSnapshotStep = new CleanupSnapshotStep(cleanSnapshotKey, deleteStepKey, client);  
   DeleteStep deleteStep = new DeleteStep(deleteStepKey, nextStepKey, client);  
   return Arrays.asList(waitForNoFollowersStep, cleanupSnapshotStep, deleteStep);  
   } else {  
   WaitForNoFollowersStep waitForNoFollowersStep = new WaitForNoFollowersStep(waitForNoFollowerStepKey, deleteStepKey, client);  
   DeleteStep deleteStep = new DeleteStep(deleteStepKey, nextStepKey, client);  
   return Arrays.asList(waitForNoFollowersStep, deleteStep);  
   }  
   }  
 

WaitForNoFollowersStep do collect indices stats to evaluate the condition

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForNoFollowersStep.java

Lines 56 to 63 in 3962556

     getClient().admin().indices().stats(request, ActionListener.wrap((response) -> {  
   IndexStats indexStats = response.getIndex(indexName);  
   if (indexStats == null) {  
   // Index was probably deleted  
   logger.debug("got null shard stats for index {}, proceeding on the assumption it has been deleted", indexName);  
   listener.onResponse(true, null);  
   return;  
   }  
 

BUT indices stats api need broadcast to many nodes and collect shardStats which is time consuming, even we know the CCR is disabled

i think, we can use feature api get feature info, and reduce indices stats request when the CCR is disabled

elasticsearchmachine · 2023-03-22T10:15:37Z

Pinging @elastic/es-data-management (Team:Data Management)

luyuncheng · 2023-04-20T12:24:02Z

@andreidan i know there are many optimizations in ilm batch task AND master batch queue like 78547, 92021, 94325 etc. BUT when many deleteAction triggered in ILM, all phrase would wait in WaitForNoFollowersStep because it should do collect shard stats from master which is time consuming, and other phase can batch execute.

andreidan

Thanks for looking into this @luyuncheng

I left an alternative solution suggestion. Hope it makes sense.

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForNoFollowersStep.java

andreidan

Thanks for working on this @luyuncheng

I've left a few rather minor suggestions (and let's see if CI is happy)

andreidan · 2023-04-27T13:12:25Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForNoFollowersStep.java

+ XPackInfoResponse.FeatureSetsInfo featureSetsInfo = xPackInfoResponse.getFeatureSetsInfo();
+ if (featureSetsInfo != null) {
+ XPackInfoResponse.FeatureSetsInfo.FeatureSet featureSet = featureSetsInfo.getFeatureSets().get(CCR_LEASE_KEY);
+ if (featureSet != null && (featureSet.available() == false || featureSet.enabled() == false)){


Let's please just check if the featureSet is enabled here.
e.g.

if (featureSet != null && featureSet.enabled() == false)

We'd like to maintain the same behaviour for this step even if CCR is unavailable (e.g. license is expired)

We'd like to maintain the same behaviour for this step even if CCR is unavailable (e.g. license is expired)

LGTM

andreidan · 2023-04-27T13:15:02Z

.../plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForNoFollowersStepTests.java

 }

- public void testConditionMet() {
+ public void testConditionMetWhenEnable() {


Shall we name this testConditionMetWhenCCREnabled ?

andreidan · 2023-04-27T13:15:20Z

.../plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForNoFollowersStepTests.java

+ assertNull(stepInfoHolder.get());
+ }
+
+ public void testConditionMetWhenDisabled() {


Similarly, shall we name this test to include CCRDisabled ?

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForNoFollowersStep.java

andreidan · 2023-04-27T13:18:15Z

@elasticmachine ok to test

andreidan · 2023-04-27T13:43:46Z

@elasticmachine update branch

…le (e.g. license is expired)

2. Add test dependency ccr for ilm 3. Add InternalClusterTests for CCR disabled

luyuncheng · 2023-04-28T08:43:10Z

@andreidan The previous ci test https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request+part-2/33165/ fail in intenalClusterTest because ActionType not found. i think this because XpackInfoFeatureAction scan all xpack module like following code shows:

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/action/TransportXPackInfoAction.java

Lines 36 to 51 in a2c9c0b

     public TransportXPackInfoAction(  
   TransportService transportService,  
   ActionFilters actionFilters,  
   LicenseService licenseService,  
   NodeClient client  
   ) {  
   super(XPackInfoAction.NAME, transportService, actionFilters, XPackInfoRequest::new);  
   this.licenseService = licenseService;  
   this.client = client;  
   this.infoActions = infoActions();  
   }  
    
   // overrideable for tests  
   protected List<XPackInfoFeatureAction> infoActions() {  
   return XPackInfoFeatureAction.ALL;  
   }  
 

and internalClusterTest in ILM not include other module into plugins when override nodePlugins:

elasticsearch/x-pack/plugin/ilm/src/internalClusterTest/java/org/elasticsearch/xpack/ilm/ClusterStateWaitThresholdBreachTests.java

Lines 69 to 72 in a2c9c0b

     @Override  
   protected Collection<Class<? extends Plugin>> nodePlugins() {  
   return Arrays.asList(LocalStateCompositeXPackPlugin.class, IndexLifecycle.class);  
   }  
 

in commits d3c5f99 i added dependency ccr into build.gradle. and i also added internalClusterTest to check this

andreidan

Thanks for iterating on this @luyuncheng

Left a few minor comments.

Can you please also add a changelog entry ? (similar to this https://github.com/elastic/elasticsearch/blob/main/docs/changelog/93524.yaml )

andreidan · 2023-05-02T09:11:15Z

...m/src/internalClusterTest/java/org/elasticsearch/xpack/ilm/ILMMultiNodeWithCCREnabledIT.java


 @ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0, numClientNodes = 0)
-public class ILMMultiNodeIT extends ESIntegTestCase {
+public class ILMMultiNodeWithCCREnabledIT extends ESIntegTestCase {


I think we don't have to rename this as it's not meant to test anything CCR related

andreidan · 2023-05-02T09:11:46Z

.../src/internalClusterTest/java/org/elasticsearch/xpack/ilm/ILMMultiNodeWithCCRDisabledIT.java

+ startWarmOnlyNode();
+ ensureGreen();
+
+ RolloverAction rolloverAction = new RolloverAction(null, null, null, 1L, null, null, null, null, null, null);


I think we can skip the hot phase completely to save a few cycles in this test

I think we can skip the hot phase completely to save a few cycles in this test

I need ShrinkAction to trigger WaitForNoFollowersStep and shrink action need a read-only index. so i combine the RolloverAction which is for enable read-only index AND ShrinkAction into one hot phase. and skip the warm phase

update master branch

luyuncheng · 2023-05-04T10:43:31Z

Can you please also add a changelog entry ?

++

andreidan · 2023-05-09T09:47:45Z

@elasticmachine update branch

andreidan

LGTM, thanks for the contribution @luyuncheng

1. Reduce WaitForNoFollowersStep requests indices shard stats

38653fa

elasticsearchmachine added v8.8.0 needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team labels Mar 15, 2023

romseygeek added :Data Management/ILM+SLM Index and Snapshot lifecycle management and removed needs:triage Requires assignment of a team area label labels Mar 22, 2023

elasticsearchmachine added the Team:Data Management Meta label for data/management team label Mar 22, 2023

andreidan self-requested a review March 23, 2023 15:39

andreidan self-assigned this Apr 21, 2023

andreidan reviewed Apr 21, 2023

View reviewed changes

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForNoFollowersStep.java Show resolved Hide resolved

gmarouli added v8.9.0 and removed v8.8.0 labels Apr 26, 2023

andreidan requested changes Apr 27, 2023

View reviewed changes

andreidan added the >enhancement label Apr 27, 2023

elasticmachine and others added 4 commits April 27, 2023 08:43

Merge branch 'main' into WaitForNoFollowersStep

712e5aa

1. format && rename test name

2f5df59

1. maintain the same behaviour for this step even if CCR is unavailab…

391e8cf

…le (e.g. license is expired)

1. Use specific ccr FeatureAction instead of XPackInfoAction

d3c5f99

2. Add test dependency ccr for ilm 3. Add InternalClusterTests for CCR disabled

andreidan reviewed May 2, 2023

View reviewed changes

luyuncheng added 3 commits May 4, 2023 16:41

Add changelog AND modified tests

7c04207

Add changelog AND modified tests AND spotless check

4d4c235

Merge pull request #4 from luyuncheng/master

77c26aa

update master branch

luyuncheng requested a review from andreidan May 9, 2023 01:31

Merge branch 'main' into WaitForNoFollowersStep

96d80c4

andreidan approved these changes May 9, 2023

View reviewed changes

andreidan merged commit 9f8593e into elastic:main May 9, 2023

arteam mentioned this pull request Nov 3, 2023

Unmute FrozenExistenceDeciderIT#testZeroToOne #101734

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reduce WaitForNoFollowersStep requests indices shard stats #94510

Reduce WaitForNoFollowersStep requests indices shard stats #94510

Uh oh!

luyuncheng commented Mar 15, 2023

elasticsearchmachine commented Mar 22, 2023

luyuncheng commented Apr 20, 2023

andreidan left a comment

Uh oh!

andreidan left a comment

andreidan Apr 27, 2023

luyuncheng Apr 27, 2023

andreidan Apr 27, 2023

luyuncheng Apr 27, 2023

andreidan Apr 27, 2023

luyuncheng Apr 27, 2023

Uh oh!

andreidan commented Apr 27, 2023

andreidan commented Apr 27, 2023

luyuncheng commented Apr 28, 2023 •

edited

Loading

andreidan left a comment

andreidan May 2, 2023

luyuncheng May 4, 2023

andreidan May 2, 2023

luyuncheng May 4, 2023

luyuncheng commented May 4, 2023

andreidan commented May 9, 2023

andreidan left a comment

Labels

6 participants

	public List<Step> toSteps(Client client, String phase, Step.StepKey nextStepKey) {
	Step.StepKey waitForNoFollowerStepKey = new Step.StepKey(phase, NAME, WaitForNoFollowersStep.NAME);
	Step.StepKey deleteStepKey = new Step.StepKey(phase, NAME, DeleteStep.NAME);
	Step.StepKey cleanSnapshotKey = new Step.StepKey(phase, NAME, CleanupSnapshotStep.NAME);

	if (deleteSearchableSnapshot) {
	WaitForNoFollowersStep waitForNoFollowersStep = new WaitForNoFollowersStep(waitForNoFollowerStepKey, cleanSnapshotKey, client);
	CleanupSnapshotStep cleanupSnapshotStep = new CleanupSnapshotStep(cleanSnapshotKey, deleteStepKey, client);
	DeleteStep deleteStep = new DeleteStep(deleteStepKey, nextStepKey, client);
	return Arrays.asList(waitForNoFollowersStep, cleanupSnapshotStep, deleteStep);
	} else {
	WaitForNoFollowersStep waitForNoFollowersStep = new WaitForNoFollowersStep(waitForNoFollowerStepKey, deleteStepKey, client);
	DeleteStep deleteStep = new DeleteStep(deleteStepKey, nextStepKey, client);
	return Arrays.asList(waitForNoFollowersStep, deleteStep);
	}
	}

	getClient().admin().indices().stats(request, ActionListener.wrap((response) -> {
	IndexStats indexStats = response.getIndex(indexName);
	if (indexStats == null) {
	// Index was probably deleted
	logger.debug("got null shard stats for index {}, proceeding on the assumption it has been deleted", indexName);
	listener.onResponse(true, null);
	return;
	}

Reduce WaitForNoFollowersStep requests indices shard stats #94510

Reduce WaitForNoFollowersStep requests indices shard stats #94510

Uh oh!

Conversation

luyuncheng commented Mar 15, 2023

elasticsearchmachine commented Mar 22, 2023

luyuncheng commented Apr 20, 2023

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

andreidan commented Apr 27, 2023

andreidan commented Apr 27, 2023

luyuncheng commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luyuncheng commented May 4, 2023

andreidan commented May 9, 2023

andreidan left a comment

Choose a reason for hiding this comment

Labels

6 participants

luyuncheng commented Apr 28, 2023 •

edited

Loading