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

[Discuss] Keep the method signature of getMaxOffsetInQueue() compatible across version 5.3.x #8924

Open
3 tasks done
LetLetMe opened this issue Nov 14, 2024 · 3 comments
Open
3 tasks done
Assignees

Comments

@LetLetMe
Copy link
Contributor

LetLetMe commented Nov 14, 2024

Before Creating the Bug Report

  • I found a bug, not just asking a question, which should be created in GitHub Discussions.

  • I have searched the GitHub Issues and GitHub Discussions of this repository and believe that this is not a duplicate.

  • I have confirmed that this bug belongs to the current repository, not other repositories of RocketMQ.

Runtime platform environment

nothing

RocketMQ version

nothing

JDK Version

nothing

Describe the Bug

image
The latest develop branch includes a method signature change that introduces a checked exception, requiring modifications across all downstream dependencies. We should discuss whether to maintain this change or roll it back temporarily, with the option to implement it consistently in a future release.

最新的develop代码中该方法签名进行了修改,抛出了checked exception,导致下游调用方都需要进行改造,该改造是否需要讨论下?看看是保留,还是先回滚等后续版本统一修改。

Steps to Reproduce

nothing

What Did You Expect to See?

nothing

What Did You See Instead?

nothing

Additional Context

No response

@LetLetMe
Copy link
Contributor Author

@lizhanhui
Copy link
Contributor

lizhanhui commented Nov 14, 2024

  1. It's reasonable to have a method raise Exception if it cannot accomplish its job and does not have enough knowledge to handle the failure properly;
  2. MessageStore cannot always be successful;
  3. If your Magic MessageStore Implementation is capable of being always successful, your job to adapt to this change should be trivial, catch and throw a runtime one or silent it;
  4. This is not a public API change beyond broker role, aka, this change does not affect clients, admin tools, etc.
  5. If you guys are worried, why not keep an eye on the pull request earlier, the pull request stayed open for several days;

@lizhanhui lizhanhui self-assigned this Nov 14, 2024
@lizhimins
Copy link
Member

lizhimins commented Nov 15, 2024

  1. Change an existing method beheavier violates the open closed principle. Moreover, if getMaxOffset method is modified, getMinOffset should also be correspondingly adjusted. Additionally, mixing the LMQ handling logic into the existing logic without proper abstraction is not elegant.
  2. It is reasonable to pay attention to and discuss this commit before the next release. As is well known, it may cost months for a Linux kernel fix to be merged into the mainline.

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

No branches or pull requests

3 participants