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

HDDS-11699. Remove unnecessary information about parts when downloading multipart files. #7558

Merged
merged 9 commits into from
Jan 9, 2025

Conversation

juncevich
Copy link
Contributor

What changes were proposed in this pull request?

HDDS-11699. Remove unnecessary information about parts when downloading multipart files

Whenever information was requested about a particular part of a multipart file, it was obtained about all parts and filtered on the Ozone Manager side. To reduce work with unnecessary information, the ability to ask for information about the current part was added.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-11699

How was this patch tested?

It was tested manually and by perf tests. Perf tests shown performance improvements:
Reduce max CPU consumption approximately 3 times.
Reduce HEAP size approximately 3 times.
Average GC pauses approximately 2 times.

@adoroszlai adoroszlai marked this pull request as draft December 11, 2024 10:06
@adoroszlai
Copy link
Contributor

@juncevich
Copy link
Contributor Author

Copy link
Contributor

@ivandika3 ivandika3 left a comment

Choose a reason for hiding this comment

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

@juncevich Thanks for the patch. Left some comments.

High level comments

  • Please add some tests to test the S3 get with part number
    • Unit tests
      • TestKeyManagerImpl to test the changes in KeyManagerImpl
    • Integration tests
      • TestOzoneRpcClientAbstract to test the getS3KeyDetails andgetS3PartKeyInfo
      • AbstractS3SDKV1Tests to test get object, specifying the part number.
        • GetObjectRequest can be specify the part number
    • Acceptance test
  • Might need to add a new OzoneManagerVersion for compatibility.
    • See how RpcClient#listStatusLight implements it
  • Please simplify the logic, the change should simply move the filtering logic from client (S3G) to server (OM) side

@juncevich juncevich force-pushed the HDDS-11699 branch 5 times, most recently from 41abc0e to 3abbf6d Compare December 13, 2024 09:26
@ivandika3 ivandika3 changed the title HDDS-11699. Remove unnecessary information about parts when downloading multipart files/ HDDS-11699. Remove unnecessary information about parts when downloading multipart files. Dec 13, 2024
@juncevich juncevich force-pushed the HDDS-11699 branch 2 times, most recently from ff132b3 to d1172c5 Compare December 24, 2024 13:26
@juncevich juncevich force-pushed the HDDS-11699 branch 2 times, most recently from a2451e2 to b7cb7ea Compare December 26, 2024 08:51
@juncevich juncevich marked this pull request as ready for review December 26, 2024 10:25
@juncevich
Copy link
Contributor Author

@ivandika3 would you re-review this pr

@ivandika3
Copy link
Contributor

@juncevich Thanks for the update.

Just looking briefly, seems #7558 (comment) has not been addressed?

I'll review the rest ASAP. Thanks again for iterating on this.

@juncevich
Copy link
Contributor Author

@juncevich Thanks for the update.

Just looking briefly, seems #7558 (comment) has not been addressed?

I'll review the rest ASAP. Thanks again for iterating on this.

Hmm, yep, you're right. I definitely fixed it. I will find a place where I lost it.

@juncevich
Copy link
Contributor Author

@juncevich Thanks for the update.

Just looking briefly, seems #7558 (comment) has not been addressed?

I'll review the rest ASAP. Thanks again for iterating on this.

Fixed. Maybe I have to recheck tests.

Copy link
Contributor

@ivandika3 ivandika3 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 the update. Overall LGTM. I left some minor comments.

Copy link
Contributor

@ivandika3 ivandika3 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 the update. LGTM +1.

Let's wait for a clean CI.

@adoroszlai
Copy link
Contributor

Let's wait for a clean CI.

Please check failures:

Error:  Tests run: 29, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 197.004 s <<< FAILURE! - in org.apache.hadoop.ozone.om.TestKeyManagerImpl
Error:  org.apache.hadoop.ozone.om.TestKeyManagerImpl.testGetNotExistedPart  Time elapsed: 0.045 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <0> but was: <5>
...
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
	at org.apache.hadoop.ozone.om.TestKeyManagerImpl.testGetNotExistedPart(TestKeyManagerImpl.java:1591)

Error:  org.apache.hadoop.ozone.om.TestKeyManagerImpl.testGetParticularPart  Time elapsed: 0.039 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <1> but was: <5>
...
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
	at org.apache.hadoop.ozone.om.TestKeyManagerImpl.testGetParticularPart(TestKeyManagerImpl.java:1567)

https://github.com/apache/ozone/actions/runs/12687113055/job/35361805439?pr=7558#step:6:4472

@juncevich
Copy link
Contributor Author

Let's wait for a clean CI.

Please check failures:

Error:  Tests run: 29, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 197.004 s <<< FAILURE! - in org.apache.hadoop.ozone.om.TestKeyManagerImpl
Error:  org.apache.hadoop.ozone.om.TestKeyManagerImpl.testGetNotExistedPart  Time elapsed: 0.045 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <0> but was: <5>
...
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
	at org.apache.hadoop.ozone.om.TestKeyManagerImpl.testGetNotExistedPart(TestKeyManagerImpl.java:1591)

Error:  org.apache.hadoop.ozone.om.TestKeyManagerImpl.testGetParticularPart  Time elapsed: 0.039 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <1> but was: <5>
...
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
	at org.apache.hadoop.ozone.om.TestKeyManagerImpl.testGetParticularPart(TestKeyManagerImpl.java:1567)

https://github.com/apache/ozone/actions/runs/12687113055/job/35361805439?pr=7558#step:6:4472

@adoroszlai, thank you for your message. Build again green.

@adoroszlai adoroszlai merged commit 9670965 into apache:master Jan 9, 2025
42 checks passed
@adoroszlai
Copy link
Contributor

Thanks @juncevich for the patch, @ivandika3 for the review.

@juncevich juncevich deleted the HDDS-11699 branch January 9, 2025 20:38
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.

3 participants