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

[ISSUE #6154] Support Amazon S3 backend in TieredStorage #6495

Merged
merged 26 commits into from
Jun 7, 2023

Conversation

TheR1sing3un
Copy link
Member

Make sure set the target branch to develop

What is the purpose of the change

fix #6154

Brief changelog

XX

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

  • Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/codeql_analysis.yml:CodeQL-Build. As part of the setup process, we have scanned this repository and found 9 existing alerts. Please check the repository Security tab to see all alerts.

@TheR1sing3un TheR1sing3un marked this pull request as ready for review March 31, 2023 09:42
@drpmma
Copy link
Contributor

drpmma commented Apr 3, 2023

It's recommended to provide more detailed documentation for such a huge commitment.

@TheR1sing3un
Copy link
Member Author

It's recommended to provide more detailed documentation for such a huge commitment.

OK! I will provide a more detailed API doc for that. I wrote the design idea in the issue #6154 and put a lot of comments in the code,maybe it can be helpful for review~

@TheR1sing3un TheR1sing3un force-pushed the support_s3_tieredstorage branch 2 times, most recently from 03e032c to 507f26d Compare April 4, 2023 14:03
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2023

Codecov Report

Attention: Patch coverage is 12.44980% with 436 lines in your changes missing coverage. Please review.

Project coverage is 42.49%. Comparing base (bee5077) to head (3400552).
Report is 503 commits behind head on develop.

Files with missing lines Patch % Lines
...tieredstore/provider/s3/TieredStorageS3Client.java 0.00% 189 Missing ⚠️
...ocketmq/tieredstore/provider/s3/S3FileSegment.java 0.00% 171 Missing ⚠️
...tieredstore/provider/s3/S3FileSegmentMetadata.java 50.64% 34 Missing and 4 partials ⚠️
...ocketmq/tieredstore/provider/s3/ChunkMetadata.java 38.09% 13 Missing ⚠️
...cketmq/tieredstore/provider/TieredFileSegment.java 37.50% 5 Missing and 5 partials ⚠️
...q/tieredstore/common/TieredMessageStoreConfig.java 50.00% 8 Missing ⚠️
...pache/rocketmq/tieredstore/TieredMessageStore.java 0.00% 2 Missing ⚠️
...ocketmq/tieredstore/container/TieredFileQueue.java 50.00% 0 Missing and 1 partial ⚠️
...mq/tieredstore/exception/TieredStoreErrorCode.java 0.00% 1 Missing ⚠️
...mq/tieredstore/exception/TieredStoreException.java 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #6495      +/-   ##
=============================================
- Coverage      42.65%   42.49%   -0.17%     
- Complexity      9093     9112      +19     
=============================================
  Files           1121     1125       +4     
  Lines          79775    80258     +483     
  Branches       10409    10453      +44     
=============================================
+ Hits           34027    34104      +77     
- Misses         41462    41853     +391     
- Partials        4286     4301      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheR1sing3un TheR1sing3un force-pushed the support_s3_tieredstorage branch from f27f48e to fc61063 Compare May 31, 2023 11:27
…rties

1. Unify all object storage configuration properties
…e expression

1. replace some lambda function with more simple expression
1. perfect comments on ChunkMetadata
1. fix unmatched config attributes in brokerS3.conf
1. More context in logging output
1. fix wrong concurrently put
1. add UT to verify TieredFileSegmentInputStream
…tter understandability

1. refactor TieredFileSegmentInputStream for better understandability
1. support `reset` of TieredFileSegmentInputStream
…commit0`

1. fix wrong position when failed in `S3FileSegment#commit0`
…e segment

1. fix still have upload buffer when already seal the segment
1. support switch to enable merge chunks into segment
1. add more debug log in TieredMessageStore
1. resolve conflicts after rebasing master

Closes apache#6624
@TheR1sing3un TheR1sing3un force-pushed the support_s3_tieredstorage branch from 0da3c6d to 050d7c8 Compare June 6, 2023 05:29
@TheR1sing3un TheR1sing3un force-pushed the support_s3_tieredstorage branch from c8a1a84 to ad10d00 Compare June 7, 2023 03:39
@zhouxinyu zhouxinyu merged commit 450d0d6 into apache:develop Jun 7, 2023
@zhouxinyu
Copy link
Member

Hi @TheR1sing3un I found some unit tests are ignored in this PR, could you please submit a PR to reopen these tests? If there is any problem passing them, please feel free to talk about it with the community.

@TheR1sing3un
Copy link
Member Author

Hi @TheR1sing3un I found some unit tests are ignored in this PR, could you please submit a PR to reopen these tests? If there is any problem passing them, please feel free to talk about it with the community.

These ut is unstable in some jdk version or machine environment, I think the reason is that : https://github.com/adobe/S3Mock which I import it in ut to mock a S3 server. I will try to fix these ut by changing dependency or using another way to test. I will submit a PR to reopen these ut soon~

@TheR1sing3un
Copy link
Member Author

Hi @TheR1sing3un I found some unit tests are ignored in this PR, could you please submit a PR to reopen these tests? If there is any problem passing them, please feel free to talk about it with the community.

Hi, I think I found the reason why bazel test always failed. Bazel build and test this project in ci with jdk1.80_242, which is incompatible for S3Mock, refer this issue: adobe/S3Mock#720 . So how about update our bazel build env to use jdk1.8.0_362 or newer version?I think newer version in jdk1.8 will deincrease incompatible dependencies

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.

Support Amazon S3 backend in TieredStorage
5 participants