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 4 commits 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.

@HoustonPutman
Copy link
Contributor Author

+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.

So I've gone ahead and added negative methods for both Compressible and Decompressible. I need to add tests for everything, but the Async APIs should be fully covered now too. Lots of ways to do everything, so lots of things to test here to make sure nothing got missed....

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.

So I've gone ahead and added negative methods for both Compressible and Decompressible. I need to add tests for everything, but the Async APIs should be fully covered now too. Lots of ways to do everything, so lots of things to test here to make sure nothing got missed....

Thank you for the hard work! The result looks good! I left some minor inline comments.

.forPath(path1, data2)
.and()
.setData()
.forPath(path2, data1)
Copy link
Member

@kezhuw kezhuw Jan 8, 2025

Choose a reason for hiding this comment

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

Suggested change
.forPath(path2, data1)
.uncompressed()
.forPath(path2, data1)

Also, I suggest to introduce new variables newData1/newData2 or reassign data1/data2. The following code is not friendly to eyeballs.

Copy link
Member

Choose a reason for hiding this comment

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

A separate test method for this case is also good from my side just like testSimple and testCreateCompressedAndUncompressed.

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.

LGTM. Thanks for your contribution!

@tisonkun tisonkun requested a review from kezhuw January 14, 2025 13:21
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