kafka consumer: Limit MaxBrokerReadBytes
#606
Conversation
Limits the `MaxBrokerReadBytes` and `MaxFetchBytes` to the maximum value that is accepted by `franz-go` library. Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
kafka/consumer.go
Outdated
| } | ||
| } | ||
| if cfg.BrokerMaxReadBytes < 0 || cfg.BrokerMaxReadBytes > 1<<30 { | ||
| cfg.BrokerMaxReadBytes = 1 << 30 |
There was a problem hiding this comment.
question: shouldn't we return an error if the value is invalid like we did for other options ?
There was a problem hiding this comment.
My rationale was that it's useful to have this limited to the max amount, since it's quite large. But perhaps we should log an info message that this is being done.
There was a problem hiding this comment.
+1 on setting to the max and logging a warning. But in alignment with @kruskall 's concerns, I don't see why this should be treated different than FetchMinBytes, MaxPollBytes and MaxPollPartitionBytes (https://github.com/elastic/apm-queue/pull/606/files#diff-bd03622ad29aac2f6cffdf1c2755ccffd0981f3e4ffef219508a265509f4fe8eR144-R152) - should we also set these to 0 if negative, and log warnings?
There was a problem hiding this comment.
@marclop I'm not sure I follow your approach to the latest change. What I tried to suggest in the above message is to go further with your approach and also align the existing code to issue log messages and set config values to the franz-kafka minimum or maximum for existing config options such as FetchMinBytes, MaxPollBytes and MaxPollPartitionBytes. I did not mean to suggest to issue an error for negative settings.
There was a problem hiding this comment.
I can do MaxPollBytes and MaxPollPartitionBytes. But FetchMinBytes doesn't really have an upper limit. We can set one arbitrarily if we wish, but I'd rather not.
Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
| cfg.Logger.Info("kafka: BrokerMaxReadBytes exceeds 1GiB, setting to 1GiB") | ||
| cfg.BrokerMaxReadBytes = 1 << 30 | ||
| } | ||
| if cfg.MaxPollBytes > 0 { |
There was a problem hiding this comment.
Should we also validate that BrokerMaxReadBytes is GTE than MaxPollBytes and set it to the bigger value otherwise?
There was a problem hiding this comment.
Isn't that what the code block above does?
There was a problem hiding this comment.
Not really, what I mean right now it's possible to set BrokerMaxReadBytes=1 and MaxPollBytes=1000000 and we will accept this configuration.
There was a problem hiding this comment.
ah good catch, added some more validation and tests.
Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
marclop
deleted the
f/add-MaxFetchBytes-and-MaxBrokerReadBytes-limits
branch
Limits the
MaxBrokerReadBytesandMaxFetchBytesto the maximum value that is accepted byfranz-golibrary.