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

Fix tiered store README.md error about Configuration #7436

Merged
merged 3 commits into from Nov 9, 2023

Conversation

golden-yang
Copy link
Contributor

Which Issue(s) This PR Fixes

ISSUE #5923 add README.md file to explain how to use tiered storage.

There was a simple configuration error

@golden-yang
Copy link
Contributor Author

@ShadowySpirits Hi, I am trying to use tiered store. Thanks for your work.

PTAL

@ShadowySpirits
Copy link
Member

@ShadowySpirits Hi, I am trying to use tiered store. Thanks for your work.

PTAL

I prefer filepath. Actually, the difference between filePath and filepath is discussed in an early issue: #6483 (comment).

@golden-yang
Copy link
Contributor Author

golden-yang commented Oct 10, 2023

@ShadowySpirits Hi, I am trying to use tiered store. Thanks for your work.
PTAL

I prefer filepath. Actually, the difference between filePath and filepath is discussed in an early issue: #6483 (comment).

I can accept both filepath and filePath . Now code and documentation are incompatible. I found change tiered store path failed in broker.conf when it isn't consistent.
Which one should we change? @ShadowySpirits

org.apache.rocketmq.tieredstore.common.TieredMessageStoreConfig#tieredStoreFilePath

@ShadowySpirits
Copy link
Member

@golden-yang I suggest to modify the code.

@golden-yang
Copy link
Contributor Author

@golden-yang I suggest to modify the code.

I change tieredStoreFilePath to tieredStoreFilepath. @ShadowySpirits

PTAL

@ShadowySpirits
Copy link
Member

@zhouxinyu Please approve the workflows.

Copy link
Contributor

@joeCarf joeCarf left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

Codecov Report

Merging #7436 (371b765) into develop (0f01df4) will increase coverage by 0.08%.
Report is 15 commits behind head on develop.
The diff coverage is 39.39%.

@@              Coverage Diff              @@
##             develop    #7436      +/-   ##
=============================================
+ Coverage      42.97%   43.06%   +0.08%     
- Complexity      9665     9686      +21     
=============================================
  Files           1161     1161              
  Lines          83920    83926       +6     
  Branches       10898    10899       +1     
=============================================
+ Hits           36064    36139      +75     
+ Misses         43379    43314      -65     
+ Partials        4477     4473       -4     
Files Coverage Δ
...q/tieredstore/common/TieredMessageStoreConfig.java 68.91% <100.00%> (ø)
...q/tieredstore/provider/posix/PosixFileSegment.java 67.56% <100.00%> (ø)
...he/rocketmq/broker/processor/PopReviveService.java 36.38% <25.00%> (ø)
...e/rocketmq/remoting/netty/NettyRemotingClient.java 40.29% <31.25%> (-0.24%) ⬇️

... and 28 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@lizhanhui lizhanhui merged commit 70dc93a into apache:develop Nov 9, 2023
9 of 10 checks passed
@lizhimins
Copy link
Member

image

@lizhimins
Copy link
Member

lizhimins commented Nov 13, 2023

There is a widely use of filePath in the rocketmq project. The rename operation which not only brings compatibility issues but also conflicts with the naming style of the rocketmq project. It is recommended to revert this change.

lizhimins added a commit to lizhimins/rocketmq that referenced this pull request Nov 14, 2023
RongtongJin pushed a commit that referenced this pull request Nov 14, 2023
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.

None yet

8 participants