Skip to content
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.

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

Improve EarlyDeprecationindexingIT test reliability #105696

Merged
merged 4 commits into from
Feb 21, 2024

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Feb 21, 2024

this test intends to test the bulkProcessor2's request consumer (see DeprecationIndexingComponent#getBulkProcessor) scheduling requests before the startup is completed (flush is enabled). To verify this behaviour the flush has to happen before the templates are loaded. To test this reliably the flush interval in the test should be as small as possible (not hardcoded 5s as of now)

This commit introduces a setting (not meant to be exposed/documented) to allow for the flush interval to be configured. It also adds additional trace logging to help with troubleshooting.

relates #104716

this test intends to test the bulkProcessor2's request consumer before the templates are initialised.
This requires the flush to happen before the templates are loaded.
To test this reliably the flush interval in the test should be as small as possible (not hardcoded 5s as of now)

This commit introduces a setting (not meant to be exposed/documented) to allow for the flush interval to be configured.
It also adds additional trace logging to help with troubleshooting.

relates elastic#104716
@pgomulka pgomulka added >test Issues or PRs that are addressing/adding tests :Core/Infra/Logging Log management and logging utilities labels Feb 21, 2024
@pgomulka pgomulka requested a review from a team February 21, 2024 13:31
@pgomulka pgomulka self-assigned this Feb 21, 2024
@pgomulka pgomulka requested a review from a team as a code owner February 21, 2024 13:31
@pgomulka pgomulka removed the request for review from a team February 21, 2024 13:31
@elasticsearchmachine elasticsearchmachine added v8.14.0 Team:Core/Infra Meta label for core/infra team labels Feb 21, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@@ -70,6 +70,7 @@ public void testEarlyDeprecationIsIndexedAfterTemplateIsLoaded() throws Exceptio
)
);
}, 30, TimeUnit.SECONDS);
fail();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a left over from testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes :) at least it would be failing reliably lol :D

setting 'xpack.security.enabled', 'false'
setting 'xpack.license.self_generated.type', 'trial'
setting 'cluster.deprecation_indexing.enabled', 'true'
setting 'cluster.deprecation_indexing.flush_interval', '1ms'
Copy link
Member

Choose a reason for hiding this comment

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

This seems infeasibly short. Would 100ms be a bit closer to real-world usage?

Copy link
Contributor Author

@pgomulka pgomulka Feb 21, 2024

Choose a reason for hiding this comment

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

the smallest the interval the better. We want the flush happen as soon as possible (to make sure it will be before the templates are loaded)
On my laptop it takes <500ms from the indexRequest being added into bulk processor to loading the templates. Could be that on CI machine it would be faster?

Copy link
Member

Choose a reason for hiding this comment

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

My only concern is that 1ms might disrupt normal ES working, but if it works - fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

disrupt normal ES working

this is the goal. indexing should not happen that early in startup so there is a queue of those too early requests to process later

one redundant log line
@thecoop
Copy link
Member

thecoop commented Feb 21, 2024

Could we put a note on #104716 to remove the trace logging once it is resolved

@pgomulka
Copy link
Contributor Author

@elasticmachine update branch

@pgomulka pgomulka merged commit 280fa40 into elastic:main Feb 21, 2024
14 checks passed
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Apr 16, 2024
this test intends to test the bulkProcessor2's request consumer (see DeprecationIndexingComponent#getBulkProcessor) scheduling requests before the startup is completed (flush is enabled). To verify this behaviour the flush has to happen before the templates are loaded. To test this reliably the flush interval in the test should be as small as possible (not hardcoded 5s as of now)

This commit introduces a setting (not meant to be exposed/documented) to allow for the flush interval to be configured. It also adds additional trace logging to help with troubleshooting.

relates elastic#104716
pgomulka added a commit that referenced this pull request Apr 16, 2024
…107540

this test intends to test the bulkProcessor2's request consumer (see DeprecationIndexingComponent#getBulkProcessor) scheduling requests before the startup is completed (flush is enabled). To verify this behaviour the flush has to happen before the templates are loaded. To test this reliably the flush interval in the test should be as small as possible (not hardcoded 5s as of now)

This commit introduces a setting (not meant to be exposed/documented) to allow for the flush interval to be configured. It also adds additional trace logging to help with troubleshooting.

relates #104716
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Logging Log management and logging utilities Team:Core/Infra Meta label for core/infra team >test Issues or PRs that are addressing/adding tests v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants