-
Notifications
You must be signed in to change notification settings - Fork 14
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
perf: avoid large offset query via limit windowing #180
Conversation
5ceee04
to
3223785
Compare
...scala/org/apache/pekko/persistence/jdbc/query/LegacyJournalDaoStreamMessagesMemoryTest.scala
Outdated
Show resolved
Hide resolved
.../src/test/scala/org/apache/pekko/persistence/jdbc/journal/dao/LimitWindowingStreamTest.scala
Show resolved
Hide resolved
.../test/scala/org/apache/pekko/persistence/jdbc/query/JournalDaoStreamMessagesMemoryTest.scala
Show resolved
Hide resolved
.../test/scala/org/apache/pekko/persistence/jdbc/query/JournalDaoStreamMessagesMemoryTest.scala
Show resolved
Hide resolved
...ain/scala/org/apache/pekko/persistence/jdbc/journal/dao/BaseJournalDaoWithReadMessages.scala
Outdated
Show resolved
Hide resolved
fromSequenceNr: Long, | ||
toSequenceNr: Long, | ||
batchSize: Int, | ||
refreshInterval: Option[(FiniteDuration, Scheduler)]) = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should add some check for fromSequenceNr
and batchSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and add checking for where fromSequenceNr > toSequenceNr
too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should add some check for fromSequenceNr and batchSize
what does that mean? batch size didn't allow zero
value, and the fromSequenceNr
was generate from acotr it self.
and add checking for where fromSequenceNr > toSequenceNr too
fromSequenceNr > toSequenceNr
is enforced down below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is generally good practice to do input validation to catch failures early and make constraints explicit
@Roiocam I want to release v1.1.0-M1 of this module within the next week or 2. Is this PR something that we can get merged in that timeframe or could we add it after v1.1.0-M1 is released? |
Busy paid time works, I will fix these over the weekend. |
lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - but I would appreciate if other people could approve it too @He-Pin @nvollmar @mdedetrich wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@Roiocam can you merge this if you are happy it is ready? This is the last PR that we need before we can do an RC for this module. |
Motivation
resolves: #179
Looks like we don't need to change a lot.
also found the legacy unit tests didn't works.