Skip to content

Conversation

@varunbharadwaj
Copy link
Contributor

@varunbharadwaj varunbharadwaj commented Nov 11, 2025

Description

  1. Update ingestion source params to be dynamic, to make it easy to tune/update consumer settings. On updating ingestion params, a new consumer will be created and start polling from the latest batchStartPointer at that time.
  2. Integration tests are refactored to extract pause and resume methods into the parent base test class.

Note: If multiple writer threads are used, it is possible that the new consumer reprocess some messages as it starts from batchStartPointer, similar to shard recovery scenario. This aligns with pull-based ingestion's atleast once message processing guarantee.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions
Copy link
Contributor

✅ Gradle check result for dfb133b: SUCCESS

@codecov
Copy link

codecov bot commented Nov 11, 2025

Codecov Report

❌ Patch coverage is 82.81250% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.19%. Comparing base (50f2231) to head (2e46e3b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...rch/indices/pollingingest/DefaultStreamPoller.java 83.72% 6 Missing and 1 partial ⚠️
...a/org/opensearch/index/engine/IngestionEngine.java 69.23% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #19963      +/-   ##
============================================
- Coverage     73.25%   73.19%   -0.07%     
+ Complexity    71615    71570      -45     
============================================
  Files          5789     5789              
  Lines        327471   327511      +40     
  Branches      47168    47170       +2     
============================================
- Hits         239905   239721     -184     
- Misses        68315    68577     +262     
+ Partials      19251    19213      -38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@varunbharadwaj varunbharadwaj marked this pull request as ready for review November 11, 2025 22:59
@varunbharadwaj varunbharadwaj requested a review from a team as a code owner November 11, 2025 22:59
@andrross
Copy link
Member

The changes will only reflect when the consumer is reinitialized on reset, or on shard reinitialization

Does this mean the update settings API will accept the new values but nothing will actually happen until the shard is somehow reinitialized? Seems potentially quite confusing as most (all?) dynamic settings in OpenSearch are applied right away.

to avoid the need for recreating index whenever consumer settings need to be updated

Is it possible to close the index, update the settings, and reopen the index as an alternative that is less painful than recreating?

@varunbharadwaj
Copy link
Contributor Author

Seems potentially quite confusing as most (all?) dynamic settings in OpenSearch are applied right away.

Makes sense, i'll see if we can add a listener and swap the consumer dynamically similar to reset API

Is it possible to close the index, update the settings, and reopen the index as an alternative that is less painful than recreating?

That should work, but will result in some downtime.

@andrross
Copy link
Member

Is it possible to close the index, update the settings, and reopen the index as an alternative that is less painful than recreating?

That should work, but will result in some downtime.

@varunbharadwaj Doesn't the approach in this PR require some downtime as well since it requires a disruptive action like reset or reinitialization to apply the new settings? If we can make them fully dynamic then that is great, it's just this in-between state where you can update them but need to do something else to make them take effect that I have concerns about.

@varunbharadwaj
Copy link
Contributor Author

varunbharadwaj commented Nov 13, 2025

@varunbharadwaj Doesn't the approach in this PR require some downtime as well since it requires a disruptive action like reset or reinitialization to apply the new settings? If we can make them fully dynamic then that is great, it's just this in-between state where you can update them but need to do something else to make them take effect that I have concerns about.

Yeah, thinking we can create a new consumer with the new settings and swap with existing consumer. There will be downtime only on indexing side. Query should not be impacted.

With the current approach, we will have to restart the nodes or relocate the shards. It should not have 100% downtime as in the case of closing the index. But I agree that it can be confusing, if it doesn't reflect immediately.

@varunbharadwaj varunbharadwaj force-pushed the vb/dynamicsetting branch 2 times, most recently from 9d56ce6 to fcf362e Compare November 19, 2025 08:18
@github-actions
Copy link
Contributor

✅ Gradle check result for fcf362e: SUCCESS

@github-actions
Copy link
Contributor

❌ Gradle check result for ff29901: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

❌ Gradle check result for ff29901: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

❌ Gradle check result for ff29901: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

✅ Gradle check result for 2e46e3b: SUCCESS

@varunbharadwaj varunbharadwaj merged commit 6f6a545 into opensearch-project:main Nov 20, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants