dongjinleekr commented on code in PR #10826:
URL: https://github.com/apache/kafka/pull/10826#discussion_r917108950
##########
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java:
##########
@@ -523,6 +532,45 @@ boolean idempotenceEnabled() {
return userConfiguredTransactions || idempotenceEnabled;
}
+ public CompressionConfig getCompressionConfig(CompressionType
compressionType) {
+ if (getString(ProducerConfig.COMPRESSION_LEVEL_CONFIG).isEmpty()) {
Review Comment:
The reason why I did not choose a simple approach like
`CompressionType.config(String level)` (as @ijuma pointed out) here is simple:
**To make room for supporting per-codec configuration in the future, like
[KIP-780](https://cwiki.apache.org/confluence/display/KAFKA/KIP-780%3A+Support+fine-grained+compression+options).**
The reason why I chose to use `compression.level` without default value (as
@mimaison pointed out) is also simple: at the point of when
[KIP-390](https://cwiki.apache.org/confluence/display/KAFKA/KIP-390%3A+Support+Compression+Level)
is passed, the proposal included global configuration option (i.e.,
`compression.level`). But, as I work on
[KIP-780](https://cwiki.apache.org/confluence/display/KAFKA/KIP-780%3A+Support+fine-grained+compression+options)
and running an [in-house](https://en.wikipedia.org/wiki/Naver_Corporation)
preview, I found that per-codec configuration (i.e.,
`compression.{gzip|lz4|zstd}.level`) is much more preferable for the following
reasons:
### 1. Easy to switch the compression type
For example, `{compression.type=zstd,compression.level=10}` works but
`{compression.type=gzip,compression.level=10}` does not, since gzip only
supports the level of 1 to 9. In other words, `compression.type` is so
error-prone when the user tries to switch the codec. (@junrao indicated a
similar problem:
[#1](https://github.com/apache/kafka/pull/10826#discussion_r850624372)
[#2](https://github.com/apache/kafka/pull/10826#discussion_r850624372))
If we limit the scope of per-codec options like `compression.gzip.level`, we
can easily switch the whole options by changing `compression.type` only.
Moreover, this approach is consistent with [additional compression
options](https://cwiki.apache.org/confluence/display/KAFKA/KIP-780%3A+Support+fine-grained+compression+options)
like `compression.gzip.buffer`.
### 2. Can provide a default level.
As @ijuma pointed out, we can not provide a default compression level with
`compression.level`; but with `compression.{gzip|lz4|zstd}.level`, **we can**.
For these reasons, I run the overall benchmarks in
[here](https://cwiki.apache.org/confluence/display/KAFKA/KIP-780%3A+Support+fine-grained+compression+options)
with per-config options (see 'Final Notes' section.) If the reviewers prefer
this scheme, I will happily change it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
