support snappyer #391

getong · 2019-12-25T07:00:20Z

No description provided.

.travis.yml

README.md

joshuawscott

We need credo to run on at least one of the builds.

getong · 2020-01-07T14:54:29Z

The credo build faild, so I delete it.

bjhaid · 2020-01-07T15:06:28Z

@getong

The credo build faild, so I delete it.

please don't delete it, instead fix the failures/warnings

getong · 2020-01-08T06:46:19Z

@bjhaid @joshuawscott
The build test is passed, please take a review of it.

lib/kafka_ex/server_0_p_10_and_later.ex

lib/kafka_ex/server_0_p_8_p_2.ex

bjhaid

Thanks lgtm

dantswain

Thanks for helping to improve KafkaEx! Overall, please try to make separate pull requests for independent changes. It makes review much easier and makes it easier to test changes independently. I think this PR could be split into at least 3 separate PRs.

As for snappyer - can you tell us why you want to make this change? I think it may be that we should just change to snappyer instead of making it an option, but I'd like to hear your motivation first.

.travis.yml

config/config.exs

dantswain · 2020-01-10T15:32:04Z

config/test.exs

@@ -1,3 +1,5 @@
 use Mix.Config

+config :kafka_ex, snappy_module: :snappyer


Again, please don't override this unless we plan to make snappyer the default. You can (and we should) test snappyer by wrapping a test where we change the config.

This is forced to use in the testenvironment and the ci environment.

This changes the config for all the tests, meaning we're not protected against regressions when using the old snappy lib.

This is problematic, because it means that the test suite wouldn't cover at all the default config we're shipping (i.e. the old snappy lib).

I suggest modifying the tests that cover compression so that they run twice, once with the default config and one with the new snappy module.

You can change the Application config for a given test (and restore it afterwards) by using Application.put_env.

do we even need to test snappyer? is this a case where we need to define a compression behavior(interface in other languages) such that this is swappable in the future without any modification to kafka_ex?

Not test snappyer

do we even need to test snappyer?

If you're talking about the lib itself, we don't. We do need to test the it integrates correctly, though right ? What I mean is: we do need to test that our code, when calling snappyer, works as expected.

Also, to re-iterate my earlier point:

most of our tests should use as much as possible the default config for the tests (unless we really need to), do be consistent with what we ship in a release.

whenever there are several configs, we should try to have at least one test checking that the changed config behaves correctly.

is this a case where we need to define a compression behavior [...] ?

Interesting idea ! I don't know much about compression in Kafka, so I don't know how useful that would be.

Addendum: the reason why I would recommend having one test with the new snappyer library is that, if we wish to make it the default for KafkaEx 1.0, we must have at least some experience with it. Including a test in the CI for snappyer allows us to get a bit of that.

Sorry just seeing this, maybe I wasn't clear, my hope is that we don't have to always have people open PRs like this in the future, we provide them with a interface they need to implement and let them go implement the interface for any new library they want to use for compression, I may be too far away from things and my suggestions might just be flat out wrong, but it seems it's just easier to empower people to change the compression library by implementing an interface rather than always opening PR to kafka_ex every time there is a new shiny/better compression library/support in the elixir ecosystem or kafka.

.travis.yml

lib/kafka_ex/server_0_p_10_and_later.ex

mix.exs

getong · 2020-01-15T01:36:29Z

@dantswain
The main reason is the snappy package does not maintain well, even the github website does not exist, when it compile with erlang 22, it has a lot of warning, and the snappyer package does maintain well. Even by the download number, the snappyer does have 92344, but snappy only has 3257, which is quite different, you can find this in https://hex.pm/packages?search=snappy&sort=recent_downloads . Both packges have the same methods. By adding this option, we can change the default package now and later.

dantswain · 2020-01-21T15:39:49Z

@getong I'm sorry it's taken me so long to get back to you on this, I've been very busy.

I think in the long term we should simply switch to using snappyer. I'd be a little concerned with doing that right away because it could be a surprise breaking change (though easily fixed). OTOH I don't like the added complexity of supporting two implementations.

@joshuawscott @bjhaid @jbruggem I think we have a few options:

Leave everything as-is for now and switch to snappyer for a 1.0 release. This means no code complexity to support both implementations, but delays the cut over.
Switch to snappyer now. This is the cleanest, rip-off-the-bandaid-est approach, but it introduces the possibility for surprise breaking changes. We can reduce the impact with documentation, and the solution to the breaking change is very simple (change a hex dependency), but it is still potentially a breaking change. Strictly speaking, semver doesn't obligate us not to introduce breaking changes sub-1.0 🤷‍♂
Take @getong's changes here which switches the default implementation to snappyer. This will probably cause all of the same complications as (2) since it would require a configuration change to revert to using snappy_erlang_nif. I think (2) is preferable to this because both are breaking changes and the solutions for end-users are similarly simple but (2) is a better path forward.
Modify @getong's changes so that snappyer is not the default and switch to snappyer at 1.0. This is the most user-friendly path forward but at the cost of code complexity now.

@getong If we go with either option (3) or (4), we will need to add tests so that both snappy and snappyer are tested. I can help you accomplish this.

getong · 2020-01-21T15:51:31Z

Make your choice and I will do it.

jbruggem · 2020-01-22T10:09:22Z

My opinion: go for option 4., since it seems supporting both doesn't involve a lot of code. It means:

we don't break current behaviour
we get to see how the new lib behaves before making it the default (via the test + via @getong and other who will probably activate the new lib in their deployments)

getong · 2020-05-21T13:51:58Z

@jbruggem
As you suggested, the code has been changed.

getong · 2020-10-17T15:22:48Z

I have update the test code, and all the test code are passed, but the coveralls upload error, this is not the test code error, I think the code can be reviewed now.

Finished in 73.7 seconds
256 tests, 0 failures
Randomized with seed 544250
** (ExCoveralls.ReportUploadError) Failed to upload the report to 'https://coveralls.io' (reason: status_code = 422, body = {"message":"service_job_id (736636358) must be unique for Travis Jobs not supplying a Coveralls Repo Token","error":true}).
    lib/excoveralls/poster.ex:21: ExCoveralls.Poster.execute/2
    (mix) lib/mix/tasks/test.ex:446: Mix.Tasks.Test.do_run/3
    (mix) lib/mix/task.ex:331: Mix.Task.run_task/3
    lib/mix/tasks.ex:54: Mix.Tasks.Coveralls.do_run/2
    (mix) lib/mix/task.ex:331: Mix.Task.run_task/3
    (mix) lib/mix/cli.ex:79: Mix.CLI.run_task/2
    (elixir) lib/code.ex:813: Code.require_file/2
The command "./scripts/ci_tests.sh" exited with 1.
cache.2
store build cache

use snappyer as default change doc

Argonus · 2021-05-13T12:04:58Z

@joshuawscott @getong
Support has been merged in: #437
This one could be closed.

joshuawscott reviewed Jan 7, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

joshuawscott reviewed Jan 7, 2020

View reviewed changes

README.md Show resolved Hide resolved

joshuawscott requested changes Jan 7, 2020

View reviewed changes

bjhaid reviewed Jan 9, 2020

View reviewed changes

lib/kafka_ex/server_0_p_10_and_later.ex Outdated Show resolved Hide resolved

joshuawscott reviewed Jan 9, 2020

View reviewed changes

lib/kafka_ex/server_0_p_8_p_2.ex Outdated Show resolved Hide resolved

bjhaid approved these changes Jan 10, 2020

View reviewed changes

dantswain requested changes Jan 10, 2020

View reviewed changes

getong added 2 commits October 17, 2020 23:27

support snappyer package

443ea26

use snappyer as default change doc

test snappyer module

34ceaf2

Argonus mentioned this pull request Feb 23, 2021

Support snappyer 2 #437

Merged

jbruggem closed this May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support snappyer #391

support snappyer #391

getong commented Dec 25, 2019

joshuawscott left a comment

getong commented Jan 7, 2020

bjhaid commented Jan 7, 2020

getong commented Jan 8, 2020

bjhaid left a comment

dantswain left a comment

dantswain Jan 10, 2020

getong Jan 11, 2020

jbruggem May 21, 2020

bjhaid May 21, 2020

getong May 22, 2020

jbruggem May 22, 2020

jbruggem May 22, 2020

bjhaid Jun 2, 2020

getong commented Jan 15, 2020

dantswain commented Jan 21, 2020

getong commented Jan 21, 2020

jbruggem commented Jan 22, 2020

getong commented May 21, 2020

getong commented Oct 17, 2020

Argonus commented May 13, 2021

		@@ -1,3 +1,5 @@
		use Mix.Config

		config :kafka_ex, snappy_module: :snappyer

support snappyer #391

support snappyer #391

Conversation

getong commented Dec 25, 2019

joshuawscott left a comment

Choose a reason for hiding this comment

getong commented Jan 7, 2020

bjhaid commented Jan 7, 2020

getong commented Jan 8, 2020

bjhaid left a comment

Choose a reason for hiding this comment

dantswain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getong commented Jan 15, 2020

dantswain commented Jan 21, 2020

getong commented Jan 21, 2020

jbruggem commented Jan 22, 2020

getong commented May 21, 2020

getong commented Oct 17, 2020

Argonus commented May 13, 2021