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

[Backport 2.x] Add recovery chunk size setting #14131

Closed
wants to merge 2 commits into from

Conversation

opensearch-trigger-bot[bot]
Copy link
Contributor

Backport 53ea952 from #13997.

Signed-off-by: Shubh Sahu <[email protected]>
(cherry picked from commit 53ea952)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks like there's a breaking API change here @gbbafna @astute-decipher. Want to make sure we're ok with it?

Copy link
Contributor

❌ Gradle check result for 42b7e87: 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?

@gbbafna
Copy link
Collaborator

gbbafna commented Jun 10, 2024

Looks like there's a breaking API change here @gbbafna @astute-decipher. Want to make sure we're ok with it?

Hi @dblock , this PR doesn't cause any breaking API change. It just adds a new setting

Sorry, my bad. It is due to this PR only . We have removed a field DEFAULT_CHUNK_SIZE which was only meant for tests to override the chunk size . In this PR we have made chunk size configurable, so we should be good with this breaking change .

However, would this cause other PRs's breaking changes workflow to fail as well ? In this case, how do we proceed ?

@dblock
Copy link
Member

dblock commented Jun 10, 2024

The only question is whether there are users that may or may no rely on this setting out there. So what makes us be sure that DEFAULT_CHUNK_SIZE was not used by anyone?

@dblock
Copy link
Member

dblock commented Jun 10, 2024

Looks like I am wrong about the backwards incompatible change being ok, see #14105 (comment). cc: @andrross, how should this PR be solving it?

…ettings.java

Co-authored-by: Andrew Ross <[email protected]>
Signed-off-by: Gaurav Bafna <[email protected]>
Copy link
Contributor

❌ Gradle check result for b058d55: 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?

Copy link
Contributor

❌ Gradle check result for b058d55:

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?

@gbbafna
Copy link
Collaborator

gbbafna commented Jun 11, 2024

Closing in favor of #14161

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