Skip to content

feat(chunkserver): add HDD_CHUNK_BULK_SIZE option#824

Merged
lgsilva3087 merged 1 commit intodevfrom
feat-chunkserver-add-HDD_CHUNK_BULK_SIZE-config-variable
Apr 8, 2026
Merged

feat(chunkserver): add HDD_CHUNK_BULK_SIZE option#824
lgsilva3087 merged 1 commit intodevfrom
feat-chunkserver-add-HDD_CHUNK_BULK_SIZE-config-variable

Conversation

@lgsilva3087
Copy link
Copy Markdown
Contributor

@lgsilva3087 lgsilva3087 commented Apr 7, 2026

Introduce a reloadable HDD_CHUNK_BULK_SIZE option for chunkserver. Use it for chunk registration and damaged/lost/new report bulks. Add default (1000) to config template, admin defaults, and docs.

LS-564

@lgsilva3087 lgsilva3087 requested a review from Copilot April 7, 2026 15:55
@lgsilva3087 lgsilva3087 self-assigned this Apr 7, 2026
@lgsilva3087 lgsilva3087 force-pushed the feat-chunkserver-add-HDD_CHUNK_BULK_SIZE-config-variable branch from 81ac1a7 to 92e2d09 Compare April 7, 2026 15:57
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new configuration option, HDD_CHUNK_BULK_SIZE, which allows users to define the number of chunks processed in a single bulk operation when reporting to the master server. This change replaces several hardcoded instances of the value 1000 with a reloadable atomic variable. The review feedback suggests enforcing a reasonable upper bound on this configuration value using cfg_get_minmaxvalue to prevent potential memory exhaustion or network communication issues caused by excessively large bulk sizes.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new reloadable chunkserver configuration option (HDD_CHUNK_BULK_SIZE) to control how many chunks are grouped per bulk when communicating chunk lists to the master, and propagates this option through code/config/docs.

Changes:

  • Introduce a reloadable HDD_CHUNK_BULK_SIZE setting (default 1000) backed by an atomic (gChunkBulkSize).
  • Use the configured bulk size for chunk registration bulks and damaged/lost/new chunk report bulks.
  • Update config template, admin default dump, and chunkserver config manpage with the new option.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/data/sfschunkserver.cfg.in Documents the new HDD_CHUNK_BULK_SIZE option in the sample config.
src/chunkserver/masterconn.cc Uses the configured bulk size for damaged/lost/new chunk report bulks.
src/chunkserver/master_connection.cc Uses the configured bulk size for registration chunk bulks.
src/chunkserver/hddspacemgr.h Adds kDefaultChunkBulkSize and atomic gChunkBulkSize; updates bulk API signature.
src/chunkserver/hddspacemgr.cc Loads/stores HDD_CHUNK_BULK_SIZE on init and reload.
src/admin/dump_config_command.cc Adds the default value for HDD_CHUNK_BULK_SIZE to chunkserver defaults.
doc/sfschunkserver.cfg.5.adoc Documents the new HDD_CHUNK_BULK_SIZE option in the manpage.

@lgsilva3087 lgsilva3087 force-pushed the feat-chunkserver-add-HDD_CHUNK_BULK_SIZE-config-variable branch from 92e2d09 to cafe7b7 Compare April 7, 2026 16:08
@lgsilva3087 lgsilva3087 requested a review from Copilot April 7, 2026 16:09
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

@lgsilva3087 lgsilva3087 marked this pull request as ready for review April 7, 2026 16:14
@lgsilva3087 lgsilva3087 force-pushed the feat-chunkserver-add-HDD_CHUNK_BULK_SIZE-config-variable branch from cafe7b7 to 9671305 Compare April 7, 2026 16:49
Copy link
Copy Markdown
Contributor

@GigaCronos GigaCronos left a comment

Choose a reason for hiding this comment

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

Looks good. Please check my suggestion. Also consider creating and linking the corresponding Linear Issue.

@lgsilva3087 lgsilva3087 force-pushed the feat-chunkserver-add-HDD_CHUNK_BULK_SIZE-config-variable branch from 9671305 to 82eba92 Compare April 7, 2026 17:09
Copy link
Copy Markdown
Contributor

@GigaCronos GigaCronos left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Copy Markdown
Contributor

@antuan96314 antuan96314 left a comment

Choose a reason for hiding this comment

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

LGTM, However, consider complementing PR with a test to make sure it works as expected.

Copy link
Copy Markdown
Contributor

@ralcolea ralcolea left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@lgsilva3087 lgsilva3087 force-pushed the feat-chunkserver-add-HDD_CHUNK_BULK_SIZE-config-variable branch from 82eba92 to f2d7c22 Compare April 8, 2026 08:51
@lgsilva3087
Copy link
Copy Markdown
Contributor Author

LGTM, However, consider complementing PR with a test to make sure it works as expected.

I added a simple one.

Introduce a reloadable HDD_CHUNK_BULK_SIZE option for chunkserver.
Use it for chunk registration and damaged/lost/new report bulks.
Add default (1000) to config template, admin defaults, and docs.

Signed-off-by: guillex <guillex@leil.io>
@lgsilva3087 lgsilva3087 force-pushed the feat-chunkserver-add-HDD_CHUNK_BULK_SIZE-config-variable branch from f2d7c22 to 3a78802 Compare April 8, 2026 11:55
@lgsilva3087 lgsilva3087 merged commit c43cb31 into dev Apr 8, 2026
10 checks passed
@lgsilva3087 lgsilva3087 deleted the feat-chunkserver-add-HDD_CHUNK_BULK_SIZE-config-variable branch April 8, 2026 16:21
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.

5 participants