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

[improve][pip] PIP-331: Replace configuration brokerServiceCompactionPhaseOneLoopTimeInSeconds with brokerServiceCompactionPhaseOneReadTimeoutInSeconds #21910

Closed
wants to merge 3 commits into from

Conversation

thetumbled
Copy link
Member

@thetumbled thetumbled commented Jan 17, 2024

Motivation

A PIP proposal to replace configuration brokerServiceCompactionPhaseOneLoopTimeInSeconds with brokerServiceCompactionPhaseOneReadTimeoutInSeconds.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions github-actions bot added type/PIP doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. labels Jan 17, 2024
@thetumbled thetumbled changed the title [improve][pip] PIP-331: Replace configuration brokerServiceCompactionPhaseOneLoopTimeInSeconds with brokerServiceCompactionPhaseOneMessageReadTimeoutInSeconds [improve][pip] PIP-331: Replace configuration brokerServiceCompactionPhaseOneLoopTimeInSeconds with brokerServiceCompactionPhaseOneReadTimeoutInSeconds Jan 17, 2024
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

Actually I don't think introducing such a config is meaningful, unless you can explain in which case will users to modify this config? Pulsar has too many configs, which is very unfriendly. i.e. I wouldn't approve #11206 if I reviewed it.

@thetumbled
Copy link
Member Author

thetumbled commented Jan 18, 2024

Actually I don't think introducing such a config is meaningful, unless you can explain in which case will users to modify this config? Pulsar has too many configs, which is very unfriendly. i.e. I wouldn't approve #11206 if I reviewed it.

This pip is just to correct the ambiguous configuration name and doc.
As for the meaning of this conf, we actually do not modify this config most of the time. But compare with reserving this misleading conf, we'd better fix it, that is rename it or remove it, it depends community's discussion.

@BewareMyPower
Copy link
Contributor

I support fixing the config documentation, which does not require a PIP. But I don't support adding a new config for that.

@thetumbled
Copy link
Member Author

I support fixing the config documentation, which does not require a PIP. But I don't support adding a new config for that.

+1

@thetumbled
Copy link
Member Author

closed for the discussion above.

@thetumbled thetumbled closed this Jan 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. type/PIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants