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.

Sign up for GitHub

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

Jump to bottom

feat: support parallel compress snapshot #2136

Merged
merged 11 commits into from
Mar 14, 2023
Merged

feat: support parallel compress snapshot #2136

merged 11 commits into from
Mar 14, 2023

Conversation

wuchaojing
Copy link
Contributor

improve #2116

@imbajin
Copy link
Member

imbajin commented Mar 1, 2023 •
edited
Loading

Hi, thanks for the contribution, for better review and reduce a lot of time in code-style problems,
It's highly recommended to use our style config if u use IDEA, import the style file and reformat your code 📑

and also fix the CI problems -> add license header & fix security problem as the CI alerts (could copy from the exist file)

@JackyYangPassion

This comment was marked as off-topic.

Sign in to view
@codecov
Copy link

codecov bot commented Mar 2, 2023 •
edited
Loading

Codecov Report

Merging #2136 (d475b22) into master (76fa644) will increase coverage by 6.06%.
The diff coverage is 19.01%.

@@             Coverage Diff              @@
##             master    #2136      +/-   ##
============================================
+ Coverage     62.60%   68.67%   +6.06%     
+ Complexity      728      676      -52     
============================================
  Files           481      491      +10     
  Lines         39704    40217     +513     
  Branches       5581     5615      +34     
============================================
+ Hits          24857    27618    +2761     
+ Misses        12389     9935    -2454     
- Partials       2458     2664     +206
Impacted Files Coverage Δ
.../store/raft/compress/ParallelCompressStrategy.java 0.00% <0.00%> (ø)
...ugegraph/backend/store/raft/StoreSnapshotFile.java 41.88% <32.00%> (-0.37%) ⬇️
...d/store/raft/compress/CompressStrategyManager.java 40.00% <40.00%> (ø)
...nd/store/raft/compress/SerialCompressStrategy.java 62.50% <62.50%> (ø)
...ache/hugegraph/backend/store/raft/RaftContext.java 81.25% <100.00%> (+0.09%) ⬆️
.../java/org/apache/hugegraph/config/CoreOptions.java 99.49% <100.00%> (+0.02%) ⬆️

... and 88 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

javeme
javeme reviewed Mar 2, 2023
View reviewed changes
Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, it's a nice enhancement.
Some tiny comments.

.../src/main/java/org/apache/hugegraph/backend/store/raft/compress/CompressStrategyManager.java Show resolved Hide resolved
.../src/main/java/org/apache/hugegraph/backend/store/raft/compress/CompressStrategyManager.java Outdated Show resolved Hide resolved
...src/main/java/org/apache/hugegraph/backend/store/raft/compress/ParallelCompressStrategy.java Show resolved Hide resolved
...ph-core/src/main/java/org/apache/hugegraph/backend/store/raft/compress/CompressStrategy.java Show resolved Hide resolved
...src/main/java/org/apache/hugegraph/backend/store/raft/compress/ParallelCompressStrategy.java Outdated Show resolved Hide resolved
@wuchaojing
Copy link
Contributor Author

Hi, thanks for the contribution, for better review and reduce a lot of time in code-style problems, It's highly recommended to use our style config if u use IDEA, import the style file and reformat your code 📑

and also fix the CI problems -> add license header & fix security problem as the CI alerts (could copy from the exist file)

Thanks for your review, code style has fixed, but fix security problem as the CI alerts I don't know how to fix it

javeme
javeme reviewed Mar 6, 2023
View reviewed changes
.../src/main/java/org/apache/hugegraph/backend/store/raft/compress/CompressStrategyManager.java Outdated
// add parallel compress strategy
if (compressStrategies[PARALLEL_STRATEGY] == null) {
final CompressStrategy compressStrategy = new ParallelCompressStrategy(
config.get(CoreOptions.RAFT_SNAPSHOT_COMPRESS_THREADS),
Copy link
Contributor

Choose a reason for hiding this comment

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

can align with "CompressStrategy" or "compressStrategy"

Copy link
Member

Choose a reason for hiding this comment

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

can align with "CompressStrategy" or "compressStrategy"

It's better not to use the manually align, it will break the auto-format style and greatly raise the threshold of contributing code (Otherwise we could define a automatic rule for it)

image

.../src/main/java/org/apache/hugegraph/backend/store/raft/compress/CompressStrategyManager.java Outdated Show resolved Hide resolved
.../src/main/java/org/apache/hugegraph/backend/store/raft/compress/CompressStrategyManager.java Outdated
config.get(CoreOptions.RAFT_SNAPSHOT_COMPRESS_THREADS),
config.get(CoreOptions.RAFT_SNAPSHOT_DECOMPRESS_THREADS));
CompressStrategyManager.addCompressStrategy(
CompressStrategyManager.PARALLEL_STRATEGY, compressStrategy);
Copy link
Contributor

Choose a reason for hiding this comment

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

align

...src/main/java/org/apache/hugegraph/backend/store/raft/compress/ParallelCompressStrategy.java Outdated Show resolved Hide resolved
.../src/main/java/org/apache/hugegraph/backend/store/raft/compress/CompressStrategyManager.java Outdated Show resolved Hide resolved
@javeme javeme added the raft label Mar 6, 2023
javeme
javeme previously approved these changes Mar 8, 2023
View reviewed changes
@javeme
Copy link
Contributor

javeme commented Mar 10, 2023

CI error:

[INFO] Running org.apache.hugegraph.api.ApiTestSuite
Error:  Tests run: 69, Failures: 32, Errors: 1, Skipped: 0, Time elapsed: 2,511.93 s <<< FAILURE! - in org.apache.hugegraph.api.ApiTestSuite
Error:  testTruncate(org.apache.hugegraph.api.GremlinApiTest)  Time elapsed: 1.173 s  <<< FAILURE!
java.lang.AssertionError: Response with status 500 and content {"exception":"java.lang.IllegalStateException","message":"The snapshot future can't be null","cause":"[java.lang.IllegalStateException]"} expected:<200> but was:<500>
	at org.apache.hugegraph.api.GremlinApiTest.testTruncate(GremlinApiTest.java:139)

@imbajin imbajin merged commit 82c7e78 into apache:master Mar 14, 2023
@imbajin imbajin linked an issue Mar 31, 2023 that may be closed by this pull request
[Question] 如何提升snapshot压缩/解压效率 #2116
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javeme javeme javeme approved these changes

@imbajin imbajin imbajin approved these changes

Assignees
No one assigned
Labels
raft
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Question] 如何提升snapshot压缩/解压效率
4 participants
@wuchaojing @imbajin @JackyYangPassion @javeme