-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
CURATOR-725: Allow for global compression #512
base: master
Are you sure you want to change the base?
Conversation
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.
+1 LGTM!
Disclaimer: I'm not a Curator internals expert
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.
+1 to global compression
When using this flag with a custom CompressionProvider, this can easily allow for a filtered compression implementation.
I saw no per-operation way to opt out compression if it is enabled global while we have method to turn it on if it is not enabled globally. People will have to override CompressionProvider
which complicate things.
I checked CreateBuilder
, SetDataBuilder
, AsyncCreateBuilder
, AsyncSetDataBuilder
and GetDataBuilder
.
I am thinking whether we can solve this by:
- Per operation
compression(bool compression)
to override global setting. I guess we probably could unifyCompressible
andDecompressible
. - Independent global filter to separate compression filter and the algorithm. This way both parts are concentrated. And we probably could make the filter composable.
public interface CompressionFilter {
boolean compressed(String path);
CompressionFilter ENABLED = path -> true;
CompressionFilter DISABLED = path -> false;
Function<Collection<CompressionFilter>, CompressionFilter> OR = filters -> path -> {
for (CompressionFilter filter : filters) {
if (filter.compressed(path)) {
return true;
}
}
return false;
};
Function<String, CompressionFilter> CHILD_NODES = parent_path -> path -> {
ZKPaths.PathAndNode nodes = ZKPaths.getPathAndNode(path);
return nodes.getPath().equals(parent_path);
};
Function<String, CompressionFilter> THIS_NODE = this_path -> path -> path.equals(this_path);
default void example() {
CompressionFilter this_or_child_nodes =
OR.apply(Arrays.asList(CHILD_NODES.apply("/xx"), THIS_NODE.apply("/xx")));
}
}
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.
Generally looks good. Thanks for your contribution!
One suggestion is that I'd prefer just name the option "compressionEnabled" and thus enableCompression
accordingly. It's configured to the factory/framework and thus bound to that scope, instead of fully global anyway.
https://issues.apache.org/jira/browse/CURATOR-725
Allow users to use a CuratorFramework setting to enable compression on all create/setData/getData requests. When using this flag with a custom
CompressionProvider
, this can easily allow for a filtered compression implementation.