zalando-nakadi / nakadi-producer-spring-boot-starter

Nakadi event producer as a Spring boot starter
MIT License
13 stars 8 forks source link

Lock-size: reasonable default or required? #182

Open ePaul opened 1 year ago

ePaul commented 1 year ago

In #153 (solving #112), we introduced the nakadi-producer.lock-size configuration property for limiting how many events are loaded into memory at once for sending out.

Back then, we left the default at "no limit" to stay compatible with the previous behavior. But having no limit can result in out-of-memory errors in the producing app, as well as overloading Nakadi with too large batches (the latter is a risk mentioned in #181). I see two options here:

Any opinion?

ePaul commented 1 year ago

An additional complication:

Currently the lock-size is injected into the EventLogRepositoryImpl instead of the transmission service, so if we make it required, it would be required even if someone uses the FES integration and disables the transmission completely, which is somewhat undesirable.

On the other hand, moving it to the transmission service (and passing as parameter to lockSomeEvents means adjusting the EventLogRepository interface, which might break someone who implements that themselves instead of using the spring-boot-starter (I know one such app, which is decommissioned since 2021), or someone who calls some of these methods directly (I've found one example in Zalando's internal github). (Or we need to introduce default methods in the interface + leave the old one there, with overriding in the implementation, like what was done for the plural persist / delete methods.)

Of course, with a major version change, both can be done.