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

Code comments of updateConsumeOffset() should be corrected #7489

Closed
2 tasks done
ghost opened this issue Oct 20, 2023 · 8 comments · Fixed by #7490
Closed
2 tasks done

Code comments of updateConsumeOffset() should be corrected #7489

ghost opened this issue Oct 20, 2023 · 8 comments · Fixed by #7490

Comments

@ghost
Copy link

ghost commented Oct 20, 2023

Search before creation

  • I had searched in the issues and found no similar issues.

Documentation Related

[org/apache/rocketmq/example/simple/PullConsumer.java]
https://github.com/apache/rocketmq/blob/develop/example/src/main/java/org/apache/rocketmq/example/simple/PullConsumer.java
image

consumer.updateConsumeOffset() uses the function of updateOffset() in OffsetStore interface here:

org/apache/rocketmq/client/consumer/store/OffsetStore.java
image

In example, the comments show "//update offset to broker" and the interface class code shows "Update the offset,store it in memory".

Or one of the two comments might be wrong. Please correct me if I misunderstood them.

Are you willing to submit PR?

  • Yes I am willing to submit a PR!
@ghost ghost added the module/doc label Oct 20, 2023
@joeCarf
Copy link
Contributor

joeCarf commented Oct 20, 2023

截屏2023-10-20 17 07 41 updateOffset这个方法有两个实现,一个是在本地存储offset,一个是将offset存在broker里。这两种实现分别对应着广播消费和集群消费模式,因此我理解这里的comment有些模糊,但是大意应该没有问题

@ghost
Copy link
Author

ghost commented Oct 20, 2023

截屏2023-10-20 17 07 41 updateOffset这个方法有两个实现,一个是在本地存储offset,一个是将offset存在broker里。这两种实现分别对应着广播消费和集群消费模式,因此我理解这里的comment有些模糊,但是大意应该没有问题

@joeCarf Thanks for your reply :)

There is no difference in the implementations of method updateOffset() between LocalFileOffsetStore and RemoteBrokerOffsetStore. The codes of updateOffset() in RemoteBrokerOffsetStore (Clustering mode as you said) are as follows:
image

As per the code, I can not found the interaction between client and broker. Therefore, I still think the comment should be corrected. You are right with the fact that the scheduled service would update offset from client memory to broker disk. However, that action is not directly to update offset to broker.

@joeCarf
Copy link
Contributor

joeCarf commented Oct 20, 2023

图中的代码只更新了内存中的offset。在RemoteBrokerOffsetStore中,有一persistAll函数,会将整个offsetTable更新到对应的broker
截屏2023-10-20 19 17 46
LocalFileOffsetStore中只会写到本地文件中

@joeCarf
Copy link
Contributor

joeCarf commented Oct 20, 2023

@tensory2022

@ghost
Copy link
Author

ghost commented Oct 23, 2023

@joeCarf

I never doubt the offset will be updated to broker, what I care is whether it is done by the method updateConsumeOffset(). If not, the code comment should be corrected.

In additon, you might need to think over this point: "而LocalFileOffsetStore中只会写到本地文件中" . Whether you use LocalFileOffsetStore or RemoteBrokerOffsetStore, the offset will be synchronized to broker by a scheduled service which runs every 10 seconds.

I hope you understand what I'm saying, thank you~

@joeCarf
Copy link
Contributor

joeCarf commented Oct 23, 2023

i get your point, and i will submit a pr to enhance it. btw, i didn't find the scheduled service which runs every 10 seconds in LocalFileOffsetStore, will you plz show me the code location

@joeCarf
Copy link
Contributor

joeCarf commented Oct 23, 2023

plz take a look at this pr #7490

@ghost
Copy link
Author

ghost commented Oct 23, 2023

@joeCarf Thanks for your PR. Hope it will be merged soon.

The code you require is at: org.apache.rocketmq.client.impl.factory.MQClientInstance.
image
And the scheduled time interval should be 5 seconds instead of 10 seconds I declared before.

RongtongJin pushed a commit that referenced this issue Oct 23, 2023
* Doc: How to debug in Idea

* en

* enhance code comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant