Skip to content

refactor(vermeer): make startChan's size configurable #328

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

Merged
merged 2 commits into from
May 30, 2025

Conversation

Ethereal-O
Copy link
Contributor

@Ethereal-O Ethereal-O commented May 22, 2025

Purpose of the PR

Use configurable options to configure the size of start_chan add error handling at the same time.
Solve problem below:
The default size of startChan is 10 and cannot be changed. The queue size may need to be manually configured by users to reduce congestion.

Main Changes

  • Add a configuration item named start_chan_size to the master.
  • Add error rollback mechanism so that default options can still be used in case of configuration errors.
  • Remind the user that they have reverted back to the default options when an exception occurs.

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. feature New feature labels May 22, 2025
@imbajin imbajin requested a review from Copilot May 22, 2025 06:07
Copy link

@Copilot 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

This PR makes the startChan buffer size configurable via a new start_chan_size option and adds error handling to fall back to a default if parsing fails.

  • Introduce start_chan_size in the master configuration
  • Read and convert the config value at startup, with fallback and logging on errors
  • Update channel initialization to use the configured size

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
vermeer/config/master.ini Add new start_chan_size configuration option
vermeer/apps/master/bl/scheduler_bl.go Load, parse, and handle errors for start_chan_size before creating startChan
Comments suppressed due to low confidence (3)

vermeer/apps/master/bl/scheduler_bl.go:21

  • Remove the unused errors import to clean up the import block.
"errors"

vermeer/apps/master/bl/scheduler_bl.go:44

  • Consider adding unit tests to cover valid and invalid start_chan_size values to verify the fallback logic works as expected.
chanSizeInt, err := strconv.Atoi(chanSize)

vermeer/apps/master/bl/scheduler_bl.go:46

  • The code uses logrus for logging but it isn't imported; add the appropriate import (e.g., github.com/sirupsen/logrus).
logrus.Errorf("failed to convert start_chan_size to int: %v", err)

auth_token_factor=1234
Copy link
Preview

Copilot AI May 22, 2025

Choose a reason for hiding this comment

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

Add a descriptive comment for start_chan_size in master.ini to explain its purpose and valid values.

Suggested change
auth_token_factor=1234
auth_token_factor=1234
; The initial size of the channel buffer used for communication between components.
; Valid values: Any positive integer. A larger value may improve performance for high-throughput systems.

Copilot uses AI. Check for mistakes.

@hankwenyx
Copy link

@imbajin This commit can be approved

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 30, 2025
@imbajin imbajin changed the title refactor(startChan): make startChan's size configurable refactor(vermeer): make startChan's size configurable May 30, 2025
@imbajin imbajin merged commit 8eb16b4 into apache:master May 30, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] make startChan's size configurable
3 participants