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

add zstd compression type #112

Merged
merged 5 commits into from
Feb 2, 2022
Merged

Conversation

lhuet
Copy link
Contributor

@lhuet lhuet commented Jan 26, 2022

Zstandard compression type is supported by the Kafka client already embedded in this plugin.
This PR allows the "zstd" compression type to be used in the Logstash Kafka output.

@roaksoax
Copy link

Fixes #2

Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

Some very minor nitpicks, but tested locally, and it does what was advertised.

@lhuet Would you mind adding changelogs, and bumping the version? As this introduces a new option, we would typically prefer a bump in the minor version.

@karenzone Would you mind checking the documentation?

DEVELOPER.md Outdated Show resolved Hide resolved
docs/output-kafka.asciidoc Outdated Show resolved Hide resolved
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Builds cleanly and renders as expected. I left very minor suggestions inline to comply with our docs standards. Thank you for helping us make our product and docs better.

Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

One minor nit on changelog format

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Rob Bavey <[email protected]>
load_kafka_data(config)
end

# NOTE: depends on zstd-ruby gem which is using a C-extension
Copy link
Contributor

@yaauie yaauie Jan 28, 2022

Choose a reason for hiding this comment

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

The spec validating the new behaviour is commented out, presumably because we need to do something else to get the C-extension compiled/installed.

What else needs to be done? If a user were to install this plugin, would zstd compression work out of the box, or would they need to side-channel load that zstd-ruby gem somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The zstd-ruby is required only for test as the Kafka ruby client is used for the tests.

The plugin itself uses the Java Kafka client that support the zstd compression out of the box.
Initially, I tested it successfully with only modifying the kafka.rb file (adding 'zstd' as 'allowed' compression type):

config :compression_type, :validate => ["none", "gzip", "snappy", "lz4", "zstd"], :default => "none"

For the LZ4, it is identical. The extlz4 gem use a C-extension that the current test suite does not support:

# NOTE: depends on extlz4 gem which is using a C-extension

As for zstd, the Java Kafka client supports the lz4 compression out of the box.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this locally, using the kafka input to pull data from the zstd compressed topic that was written to by the updated kafka output, given that the zstd integration with the ruby-kafka gem that is used for integration tests only uses a gem without jruby support.

Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM

@robbavey
Copy link
Contributor

robbavey commented Feb 1, 2022

@karenzone I've approved the code portion, are the docs good to go?

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

LGTM

@robbavey
Copy link
Contributor

robbavey commented Feb 2, 2022

@lhuet This is good to go. Thank you for your contribution! I will update this ticket when the publishing process is complete

@robbavey robbavey merged commit e5bf126 into logstash-plugins:main Feb 2, 2022
@robbavey
Copy link
Contributor

robbavey commented Feb 2, 2022

This plugin has now been published as v10.10.0 of the kafka integration plugin

edefaria pushed a commit to edefaria/logstash-integration-kafka that referenced this pull request Feb 3, 2023
Merge in THOT/logstash-integration-kafka from dev-edefaria-siem to siem

* commit 'd66d027404b5afd6e68e282f669a33e45d3cb392':
  update multiple dependancy and allow to build with jdk18
  Update kafka version for integration tests (logstash-plugins#123)
  [Doc] Improve doc for keystore types (logstash-plugins#121)
  Describe potenatial problems in `topic_pattern` setting (logstash-plugins#122)
  Fix CHANGELOG.md link to PR (logstash-plugins#119)
  bump kafka client to 2.8.1 (logstash-plugins#115)
  Feat: added connections_max_idle_ms setting for output (logstash-plugins#118)
  Update CHANGELOG.md (logstash-plugins#114)
  add zstd compression type (logstash-plugins#112)
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.

5 participants