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

Rust compaction #1360

Merged
merged 170 commits into from
Jul 12, 2024
Merged

Rust compaction #1360

merged 170 commits into from
Jul 12, 2024

Conversation

m09526
Copy link
Member

@m09526 m09526 commented Sep 22, 2023 •
edited by patchwork01
Loading

THIS IS AN EXPERIMENTAL PR. NOT READY FOR MERGE.

Issue

Tests

Rust compaction units tests not present yet.

@m09526 m09526 linked an issue Sep 22, 2023 that may be closed by this pull request
Accelerated compaction support #1359
Closed
@m09526 m09526 requested review from patchwork01, kr565370 and gaffer01 and removed request for patchwork01, kr565370 and gaffer01 September 22, 2023 12:57
@gaffer01
Copy link
Member

gaffer01 commented Sep 26, 2023 •
edited
Loading

It takes 20 minutes to to build this PR (compared to around 4 without the Rust build) and it takes 20 minutes every time. Is there any way to avoid recompiling the Rust code if nothing has changed?

We will need both unit tests in the Rust code, and the Java unit tests to test both the Java and Rust implementations.

The actual compaction code doesn't seem to be sending many logs to CloudWatch - it's harder to see that the job is processing than it is with the Java implementation - there are no logs of read 1000000 records, read 2000000 records, ...:

[main] compaction.jobexecution.RustBridge DEBUG  - Attempting to load native library from JAR path natives/release
[main] compaction.jobexecution.RustBridge DEBUG  - Attempting to load native library from JAR path natives/x86_64-unknown-linux-gnu/release
[main] compaction.jobexecution.RustBridge DEBUG  - Extracted file is at /tmp/nativelib-loader_14764728764528488113/libcompaction.so
2023-09-26T12:11:32 [INFO] - load_region; provider=EnvironmentVariableRegionProvider { env: Env(Real) }
[Thread-0] common.action.ChangeMessageVisibilityTimeoutAction INFO - Compaction job...

More comments to come.

gaffer01
gaffer01 reviewed Sep 26, 2023
View reviewed changes
...compaction/compaction-rust/src/main/java/sleeper/compaction/jobexecution/RustCompaction.java Outdated
RustBridge.FFICompactionResult compactionData = nativeLib.allocate_result();

try {
// Perform compaction
Copy link
Member

Choose a reason for hiding this comment

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

Add log line here to say about to run compaction using Rust.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

gaffer01
gaffer01 reviewed Sep 26, 2023
View reviewed changes
...mpaction-job-execution/src/main/java/sleeper/compaction/jobexecution/CompactSortedFiles.java Outdated
recordsProcessed = compactNoSplitting();
// If using non-default method, try that and fallback to default if it fails,
// otherwise, just use default
if (method != CompactionMethod.DEFAULT) {
Copy link
Member

Choose a reason for hiding this comment

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

Add log lines to indicate which methods are being used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@patchwork01
Copy link
Collaborator

I've split out a separate issue to improve test coverage for these changes, so that we can hopefully get this PR merged close to as it is now:

patchwork01
patchwork01 reviewed Jul 10, 2024
View reviewed changes
.github/workflows/chunk.yaml Show resolved Hide resolved
.github/workflows/docker-cli-image.yaml Outdated Show resolved Hide resolved
.github/workflows/dependency-check.yaml Outdated Show resolved Hide resolved
@patchwork01 patchwork01 marked this pull request as ready for review July 11, 2024 13:41
@patchwork01 patchwork01 merged commit 2427ac8 into develop Jul 12, 2024
20 checks passed
@patchwork01 patchwork01 deleted the 1359-accelerated-compaction-support branch July 12, 2024 08:17
@patchwork01 patchwork01 mentioned this pull request Jul 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaffer01 gaffer01 gaffer01 left review comments

@kr565370 kr565370 kr565370 approved these changes

@patchwork01 patchwork01 patchwork01 approved these changes

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

Successfully merging this pull request may close these issues.

Accelerated compaction support
4 participants
@m09526 @gaffer01 @patchwork01 @kr565370