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

Handle database replication delay when adding todos #93

Merged
merged 1 commit into from
Jul 30, 2020

Conversation

gabrieljackson
Copy link
Contributor

This change adds logic to ensure newly-added todos are in the
returned list. When the Mattermost server running the plugin has
database read replicas configured, it is possible that the plugin
would miss listing the new todo due to replication delay. This
plugin logic attempts to address the issue when it is detected.

I verified this issue and fix by using a dev server with a database
with read replicas configured.

When writing a fix, I went with something that felt like a good
trade-off on code complexity and speed. i.e. I avoided a simple
sleep, but also didn't want to drastically refactor code to fix
the issue in a slightly-better way. All that said, let me know what
you think.

Fixes #87

This change adds logic to ensure newly-added todos are in the
returned list. When the Mattermost server running the plugin has
database read replicas configured, it is possible that the plugin
would miss listing the new todo due to replication delay. This
plugin logic attempts to address the issue when it is detected.
@gabrieljackson gabrieljackson added the 2: Dev Review Requires review by a core committer label Jul 12, 2020
@gabrieljackson gabrieljackson requested review from larkox and hanzei July 12, 2020 00:09
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2020

Codecov Report

Merging #93 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #93   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           8       8           
  Lines         858     868   +10     
======================================
- Misses        858     868   +10     
Impacted Files Coverage Δ
server/command.go 0.00% <0.00%> (ø)
server/list.go 0.00% <0.00%> (ø)
server/plugin.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 889ce94...cb03cc1. Read the comment docs.

Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM! Simple and elegant.

Thank you very much for the fix 😄

@larkox larkox requested review from mickmister and removed request for hanzei July 13, 2020 08:48
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

LGTM!

@mickmister mickmister added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Jul 13, 2020
@mickmister mickmister requested a review from DHaussermann July 13, 2020 15:04
@mickmister
Copy link
Contributor

@DHaussermann You may not be able to reproduce the initial issue due to needing the db replica setup. I think this will just need a general smoke test.

@gabrieljackson
Copy link
Contributor Author

Yeah, even once I had a proper test environment, the issue took many /todo add commands to manifest itself. It definitely is tricky to find and verify outside of something like our community server.

@gabrieljackson
Copy link
Contributor Author

Just checking that this isn't waiting on me for anything.

cc. @DHaussermann

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed
Regression tested the plugin for proper functionality

  • Various cases of creating and sending ToDos
  • Creating from a post and adding to thread are working
  • All DMs form ToDo bot based on other user actions are working as expected
  • Slash commands are working
    LGTM!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jul 29, 2020
@larkox larkox merged commit ff178b2 into master Jul 30, 2020
@larkox larkox deleted the add-issue-replica-delay-fix branch July 30, 2020 07:33
@larkox larkox mentioned this pull request Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding todo via /todo add doesn't show newly added item after sending the message
5 participants