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

feat: Add delta compression support #74

Merged
merged 1 commit into from
Oct 5, 2024
Merged

Conversation

itismoej
Copy link
Contributor

@itismoej itismoej commented Sep 30, 2024

Introduce a way to set new Subscription delta option:

SubscriptionOptions subOpts = new SubscriptionOptions();
subOpts.setDelta("fossil")  // <=== HERE
Subscription sub = client.newSubscription("chat:index", subOpts, subListener);

Option delta can only have fossil value now (delta compression based on Fossil algorithm).

When set, client will try to negotiate delta compression in channel with a server. If server allows this delta type – messages in channel MAY be delta-compressed. Whether messages are delta compressed or not depends on server publishing logic.

@FZambia
Copy link
Member

FZambia commented Oct 1, 2024

Hello @itismoej

Many thanks, this is awesome to get such contributions!

I will review changes very soon, a couple of questions:

  1. Just curious, do you work in the same team with @Mahdibn ? Or it's just a coincidence that Implemented Delta Fossil Compression centrifuge-swift#104 is opened almost at the same time ? :)
  2. Could you please also add additional test cases from https://github.com/dchest/fossil-delta-js/tree/master/test ? I have not looked closer yet, maybe you already using them, but if not - would be nice to have here, as it's hard to validate the algorithm correctness during the review.

@itismoej
Copy link
Contributor Author

itismoej commented Oct 1, 2024

  1. Just curious, do you work in the same team with @Mahdibn ? Or it's just a coincidence that Implemented Delta Fossil Compression centrifuge-swift#104 is opened almost at the same time ? :)
  2. Could you please also add additional test cases from https://github.com/dchest/fossil-delta-js/tree/master/test ? I have not looked closer yet, maybe you already using them, but if not - would be nice to have here, as it's hard to validate the algorithm correctness during the review.

Hi Alex,

  1. Yes we work together but not on the same team :)
  2. Sure. I've already added some test-cases but will add your tests too. Will do this in the PR that I created for the python client too. But, since I have not included the createDelta functionality in the client code as it's not being used, I will not include the tests related to it neither.

@itismoej
Copy link
Contributor Author

itismoej commented Oct 1, 2024

@itismoej
Copy link
Contributor Author

itismoej commented Oct 1, 2024

It's weird that the tests didn't pass!

➜  centrifuge-java git:(master) ./gradlew --version       

------------------------------------------------------------
Gradle 7.5.1
------------------------------------------------------------

Build time:   2022-08-05 21:17:56 UTC
Revision:     d1daa0cbf1a0103000b71484e1dbfe096e095918

Kotlin:       1.6.21
Groovy:       3.0.10
Ant:          Apache Ant(TM) version 1.10.11 compiled on July 10 2021
JVM:          11.0.24 (Eclipse Adoptium 11.0.24+8)
OS:           Linux 6.8.0-45-generic amd64

➜  centrifuge-java git:(master) java --version
openjdk 11.0.24 2024-07-16
OpenJDK Runtime Environment Temurin-11.0.24+8 (build 11.0.24+8)
OpenJDK 64-Bit Server VM Temurin-11.0.24+8 (build 11.0.24+8, mixed mode)
➜  centrifuge-java git:(master) ./gradlew :centrifuge:test

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.5.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 1s
10 actionable tasks: 10 up-to-date
➜  centrifuge-java git:(master)

@FZambia
Copy link
Member

FZambia commented Oct 1, 2024

Yep, strange. Probably somehow related to current working directory used during test run in CI and the resolution of file paths to load content from.

@itismoej
Copy link
Contributor Author

itismoej commented Oct 1, 2024

Changed to relative path. Maybe that helps!

@FZambia
Copy link
Member

FZambia commented Oct 1, 2024

One more shot in the dark - maybe return the paths you had and try disabling gradle cache

?

Unfortunately, I don't understand how testing in Java works under the hood to give a proper advice :(

@FZambia
Copy link
Member

FZambia commented Oct 1, 2024

try disabling gradle cache

Nope, it won't help.

@FZambia
Copy link
Member

FZambia commented Oct 1, 2024

@itismoej please pay attention to target files - I see them in tests, but such files are gitignored - and they are missing in PR due to that.

@itismoej
Copy link
Contributor Author

itismoej commented Oct 1, 2024

@itismoej please pay attention to target files - I see them in tests, but such files are gitignored - and they are missing in PR due to that.

Oh! Thanks Alex, I wouldn't notice at all :))

Fixed it.

@FZambia
Copy link
Member

FZambia commented Oct 2, 2024

I do not see anything else important to tweak here at this point – looks good 👍 I'll now experiment locally a bit, maybe come with some comments after that. Will try to do this till the end of the week.

@FZambia FZambia merged commit 3e0a8c4 into centrifugal:master Oct 5, 2024
4 checks passed
@FZambia
Copy link
Member

FZambia commented Oct 5, 2024

Hello @itismoej , here is an update:

I am planning next to look at centrifugal/centrifuge-python#20 - once it is merged, will proceed with releases of both SDKs - Java and Python. It's better for me because the feature is quite complex and while reviewigng Python lib I can realize sth to be tweaked here. And before SDKs releases Centrifugo with the mentioned fix should be already available.

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.

2 participants