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

CURATOR-725: Allow for global compression #512

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HoustonPutman
Copy link
Contributor

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.

Copy link

@dsmiley dsmiley left a 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

Copy link
Member

@kezhuw kezhuw left a 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:

  1. Per operation compression(bool compression) to override global setting. I guess we probably could unify Compressible and Decompressible.
  2. 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")));
    }
}

Copy link
Member

@tisonkun tisonkun left a 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.

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.

4 participants