Skip to content

Conversation

@ricellis
Copy link
Member

@ricellis ricellis commented Apr 4, 2025

PR summary

Use normal feed mode for changes follower one-off FINITE mode.

Fixes: s1116 / i445

Note: An existing issue is required before opening a PR.

PR Checklist

Please make sure that your PR fulfills the following requirements:

  • The commit message follows the
    Angular Commit Message Guidelines.
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • New tests
  • Build/CI related changes
  • Documentation content changes
  • Other (please describe) - performance improvement

What is the current behavior?

Currently the changes follower uses longpoll feed type is for both start (LISTEN) and startOneOff (FINITE) cases.
If the number of changes is an exact multiple of the follower batch size this causes the one-off collection to unnecessarily wait for the longpoll period (57 s) when it reaches the end of the available changes. This is a rare occurrence, but one more common edge case is empty databases (i.e. the zero multiple).

What is the new behavior?

Switch to use the normal feed type for startOneOff (FINITE) mode thereby optimizing the startOneOff cases to avoid waiting unnecessarily.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@ricellis ricellis self-assigned this Apr 4, 2025
@eiri eiri self-requested a review April 4, 2025 16:27
}
if (invalidOptions.size() > 0) {
throwInvalidOptionsMessageWith(String.format(" when using %s.", ChangesFollower.class.getSimpleName()), invalidOptions);
throwInvalidOptionsMessageWith(invalidOptions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No against it, but what's a rational behind this change?

Copy link
Member Author

@ricellis ricellis Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was obsolete, I think the message was originally formatted here, but later it was adjusted and the suffix param of throwInvalidOptionsMessageWith was no longer used, but it wasn't removed. This just cleans up that oversight.

Copy link
Contributor

@eiri eiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks legit to me

@ricellis ricellis force-pushed the i445-follower-one-off branch from 4c75eee to 60be197 Compare April 22, 2025 11:02
@ricellis ricellis merged commit 60db3b2 into main Apr 22, 2025
8 checks passed
@ricellis ricellis deleted the i445-follower-one-off branch April 22, 2025 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants