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

Modified cluster.yaml to support num servers per group #5037

Merged
merged 8 commits into from
Nov 9, 2024

Conversation

dlmarion
Copy link
Contributor

@dlmarion dlmarion commented Nov 6, 2024

Modified cluster.yaml to support specifying number servers per group for compactors, sservers, and tservers. This was supported previously, but the implementation was different and the information was not as tightly coupled in the yaml file.

Closes #5032

Modified cluster.yaml to support specifying number servers
per group for compactors, sservers, and tservers. This was
supported previously, but the implementation was different
and the information was not as tightly coupled in the yaml
file.

Closes apache#5032
@dlmarion dlmarion added this to the 4.0.0 milestone Nov 6, 2024
@dlmarion dlmarion self-assigned this Nov 6, 2024
@dlmarion
Copy link
Contributor Author

dlmarion commented Nov 6, 2024

Draft because I have not tested locally yet.

@dlmarion
Copy link
Contributor Author

dlmarion commented Nov 6, 2024

I'm not sure what change shfmt is suggesting I make. I looked for tabs in the file and did not see any.

Copy link
Contributor

@keith-turner keith-turner left a comment

Choose a reason for hiding this comment

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

These changes are really nice. I only looked at the java code and java test in detail though. Did not look at the shell code changes in detail.

@keith-turner
Copy link
Contributor

Would a 2.1 cluster.yaml file fail to parse w/ a clean message w/ these changes?

@dlmarion
Copy link
Contributor Author

dlmarion commented Nov 8, 2024

Would a 2.1 cluster.yaml file fail to parse w/ a clean message w/ these changes?

I don't think the compactor/sserver/tserver sections would work. I can test with a 2.1 config and make sure a good error is thrown.

tserver:
default:
- localhost

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also have a compaction-coordinator section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, scan servers and external compactions are optional and experimental in 2.1. This is a minimal config for 2.1

Copy link
Contributor

@ddanielr ddanielr left a comment

Choose a reason for hiding this comment

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

Tested locally.
Specified different groups and number of servers per group.
stop and start commands worked.
Verified service counts via ./accumulo admin serviceStatus --noHosts

@dlmarion dlmarion marked this pull request as ready for review November 9, 2024 14:00
@dlmarion dlmarion merged commit 01a89f1 into apache:main Nov 9, 2024
8 checks passed
@dlmarion dlmarion deleted the 5032-num-hosts-per-group branch November 9, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

cluster.yaml tservers_per_host does not account for resource groups
3 participants