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

fix: change cloudant.since logic to prefer saved offset, if present #125

Merged
merged 7 commits into from
Oct 27, 2022

Conversation

tomblench
Copy link
Contributor

fixes #124

This was preventing the connector from resuming properly on restarts.

If the last sequence number is not present in offset storage, _then_ 0
will be used.

Tested manually be restarting and observing log message and lack of
event duplication.

fixes #124
Copy link
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

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

The change seems fine, but I wonder if we should do three additional things here, namely:

  1. Update
    CloudantLastSeqNumDoc = Last update sequence identifier to resume from. \
    to indicate what the behaviour is when unset (since it will no longer show a default value of "0" I think).
  2. Regenerate the config doc since the default is now different (and to include above update).
  3. Add a test so we don't accidentally regress this at some point.

@tomblench
Copy link
Contributor Author

The change seems fine, but I wonder if we should do three additional things here, namely:

  1. Update
    CloudantLastSeqNumDoc = Last update sequence identifier to resume from. \

    to indicate what the behaviour is when unset (since it will no longer show a default value of "0" I think).
  2. Regenerate the config doc since the default is now different (and to include above update).
  3. Add a test so we don't accidentally regress this at some point.
  1. could you please look at 64386b4 - i'm not totally sure what the message should look like here so i'm open to suggestions (eg should it be more explicit that this behaviour is used to track the latest sequence between restarts)
  2. sure
  3. just a unit test for the defaults? that should be pretty easy

- Prefer value from offset storage if present
- Clarify docs
@tomblench tomblench force-pushed the 124-fix-cloudant-since-startup branch from 9e9d9d4 to 47d1533 Compare October 26, 2022 14:53
@tomblench tomblench changed the title fix: don't default cloudant.since to 0 fix: change cloudant.since logic to prefer saved offset, if present Oct 26, 2022
Copy link
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

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

+1, small nit

tomblench and others added 2 commits October 27, 2022 09:43
Co-authored-by: Rich Ellis <ricellis@users.noreply.github.com>
also remove redundant constant `DEFAULT_CLOUDANT_LAST_SEQ`
Copy link
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

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

+1 a couple of nits on copy/paste test comments

@ricellis ricellis added this to the 0.200.0 milestone Oct 27, 2022
Co-authored-by: Rich Ellis <ricellis@users.noreply.github.com>
@tomblench
Copy link
Contributor Author

thanks @ricellis for spotting those copy/pastes

@tomblench tomblench merged commit 223b60c into main Oct 27, 2022
@tomblench tomblench deleted the 124-fix-cloudant-since-startup branch October 27, 2022 11:54
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.

cloudant.since defaults to 0 which means the connector always starts from the beginning on restarts
2 participants