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

Transactional Batch Saving #3829

Merged
merged 54 commits into from
Mar 14, 2019
Merged

Transactional Batch Saving #3829

merged 54 commits into from
Mar 14, 2019

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Feb 26, 2019

URL of deployed dev instance (used for testing):

Steps to test:

  • drag large NML into wK
  • save, leave page before it reaches 100%
  • reload page, should get old or new tracing (not something in between)
  • kill backend during saving
  • retry should either continue seamlessly or show conflict and the upcoming reload should give the old (consistent) tracing – (which of these two depends on the timing of the backend kill)
  • the only way to still get inconsistent tracings, i think, is to kill the backend during the committing phase when it received the final request of a transaction
  • when having a tracing open in two tabs, one (A) saving a large NML, and one (B) saving a small change after A has already successfully sent some requests, the small change (B) should still win (commits first) and the batched large saving process from A should fail when committing (refresh should then yield the small change from B)
  • normal saving should also still work (both skeleton + volume)
  • normal saving combined with a large transaction should also work

Issues:

TODO

  • sync batch cache to disk (redis?)
  • ignore duplicate requests properly again (see Silently ignore duplicate tracing save requests #3767 )
  • clean up code, extract functions?
  • remove redundant request id
  • add config for redis address
  • in health check, try redis
  • redis error handling?
  • update readme (redis is now required)
  • discuss if progress percentage should be adapted (committing requests are significantly slower on the server) (→for now, let’s leave it as is)
  • fix front-end tests
  • include redis in docker files
  • adapt infrastructure setup to include redis
  • test health on a dev instance
  • test all the scenarios
  • after testing, remove (some of) the debug output

@fm3 fm3 added the backend label Feb 27, 2019
@fm3 fm3 changed the title [WIP] Save batch transactions [WIP] Transactional Batch Saving Feb 28, 2019
@fm3 fm3 requested a review from jstriebel March 6, 2019 15:02
@fm3 fm3 changed the title [WIP] Transactional Batch Saving Transactional Batch Saving Mar 6, 2019
@fm3
Copy link
Member Author

fm3 commented Mar 6, 2019

@jstriebel could I ask you to look into adding redis to the docker files so that we can eventually use this in production? :) I’d also be grateful for a first review/look at the backend changes. I realize the code is rather complex and I’d be happy to discuss it further

@philippotto Could you have a look at the frontend tests? :)

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@fm3
Copy link
Member Author

fm3 commented Mar 13, 2019

@jstriebel I think all checkboxes are ticked now, would you like to go for a final review round? :)

Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

@fm3 I had another quick look, LGTM so far, just left some minor comments 👍

In the backend code I only checked RedisTemporaryStore.scala in detail as it changed with the withExceptionHandler. Is there anything else that I should review once more in detail? And I guess somebody should also have a look at the frontend changes.

README.md Outdated Show resolved Hide resolved
conf/application.conf Outdated Show resolved Hide resolved
Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

👍 everything fine from my side

Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

👍 everything fine from my side

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Frontend looks good 👍

frontend/javascripts/oxalis/model/sagas/save_saga.js Outdated Show resolved Hide resolved
@fm3 fm3 merged commit bd8d374 into master Mar 14, 2019
@fm3 fm3 deleted the save-batch-transactions branch March 14, 2019 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

saving skeletons: ensure that update action batches are saved transactionally
4 participants