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

Lock-size: reasonable default or required? #182

Open
ePaul opened this issue Sep 12, 2023 · 1 comment
Open

Lock-size: reasonable default or required? #182

ePaul opened this issue Sep 12, 2023 · 1 comment
Labels
auto-configuration everything about the auto-configuration features nakadi-submission persistence everything around DB access question

Comments

@ePaul
Copy link
Member

ePaul commented Sep 12, 2023

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:

  • Make the configuration option required.

    • This way, every developer is forced to think about what the right limit would be.
    • This is an incompatible change, in the sense that previously working configuration might not start up anymore.
    • So we should only do that when we release a new major version anyways.
  • Introduce an arbitrary limit (like 1000)

    • For some applications/event types with huge events, this might still be too high (OOM before reaching the limit, or Kafka timeout on Nakadi side).
    • For others with tiny events, this might be too low (not enough throughput due to the limit).

Any opinion?

@ePaul ePaul added question auto-configuration everything about the auto-configuration features nakadi-submission persistence everything around DB access labels Sep 12, 2023
@ePaul
Copy link
Member Author

ePaul commented Sep 12, 2023

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.

ePaul added a commit that referenced this issue Sep 12, 2023
This is one option from #182.
ePaul added a commit that referenced this issue Sep 12, 2023
This is one option from #182.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-configuration everything about the auto-configuration features nakadi-submission persistence everything around DB access question
Projects
None yet
Development

No branches or pull requests

1 participant