-
Notifications
You must be signed in to change notification settings - Fork 262
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
feat(java): support meta compression by Deflater #1663
Conversation
} | ||
byte[] compressed = | ||
classResolver | ||
.getFury() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had an idea when I was looking at the code recently.
Could org.apache.fury.config.Config
be set to org.apache.fury.resolver.SerializationContext
?
I think it is more reasonable to get Config
from SerializationContext
during runtime. It feels a bit weird to get it from Fury
object.
This will reduce the burden on the Fury
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, I like the idea to get config from context. But other get context, user must get fury first, then get config from context. It will introduce an extra indirection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not related to this PR, we can discuss it in another issue
java/fury-core/src/main/java/org/apache/fury/config/FuryBuilder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What does this PR do?
This PR support meta compression and add Deflater as default compressor.
In our test, it can compress meta by reduce size of 243 without introducing any performance cost:
Related issues
#1660
Does this PR introduce any user-facing change?
Benchmark