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

feat(regionserver): add graceful shutdown configuration #570

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

razvan
Copy link
Member

@razvan razvan commented Oct 2, 2024

Description

part of: #508

requires: stackabletech/docker-images#898

Implementation for "approach B" in https://github.com/stackabletech/decisions/issues/30

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

Preview Give feedback

Reviewer

Preview Give feedback

Acceptance

Preview Give feedback

@razvan razvan marked this pull request as ready for review October 14, 2024 16:24
@razvan razvan requested a review from a team October 14, 2024 16:24
@razvan razvan self-assigned this Oct 15, 2024
@razvan razvan marked this pull request as draft October 15, 2024 15:53
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Partial review

extraOpts: ["--designatedFile", "/path/to/designatedFile"] <4>
----
<1>: Run the region mover tool before shutting down the region server. Default is `false`.
<2>: Maximum number of threads to use for moving regions. Default is 1.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a hint how this value should be set? Eg: number of servers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose it depends on the number of regions, servers to distribute the regions to, total row count and what not.

As usual : it's complicated and if you don't have performance problems, don't touch it!

"1" is the default region mover it's self uses.

Copy link
Member

Choose a reason for hiding this comment

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

In any case, there probably should be some guidance rather than leaving it ambiguous.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have to defer to the experts then.

<1>: Run the region mover tool before shutting down the region server. Default is `false`.
<2>: Maximum number of threads to use for moving regions. Default is 1.
<3>: Enable or disable region confirmation on the present and target servers. Default is `true`.
<4>: Extra options to pass to the region mover tool.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a link to valid options (or maybe they're covered already?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find any documentation on this tool. I used the source.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we ca give a hint where to find the extra options in the source code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done: 7e118ab

rust/crd/src/affinity.rs Show resolved Hide resolved
rust/crd/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Sebastian Bernauer <[email protected]>
@razvan
Copy link
Member Author

razvan commented Oct 23, 2024

One rather important thing is that we should use the Fragment mechanism for the RegionMover struct.

Not disagreeing, but what is the benefit against just implementing Merge for it (as it's already done) ?

It seems to me like a lot more (quirky) code would be needed to achieve the same thing.

tests/test-definition.yaml Outdated Show resolved Hide resolved
tests/templates/kuttl/shutdown/00-limit-range.yaml Outdated Show resolved Hide resolved
rust/crd/src/lib.rs Outdated Show resolved Hide resolved
rust/crd/src/lib.rs Outdated Show resolved Hide resolved
rust/operator-binary/src/hbase_controller.rs Show resolved Hide resolved
rust/operator-binary/src/hbase_controller.rs Show resolved Hide resolved
@razvan razvan requested a review from siegfriedweber October 28, 2024 13:21
rust/crd/src/lib.rs Outdated Show resolved Hide resolved
rust/crd/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@siegfriedweber siegfriedweber left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Development: In Review
Development

Successfully merging this pull request may close these issues.

4 participants