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

Saga Recovery #27

Merged
merged 5 commits into from
Jun 16, 2016
Merged

Saga Recovery #27

merged 5 commits into from
Jun 16, 2016

Conversation

CaitieM20
Copy link
Contributor

Saga Recovery will happen at Startup. Once a Scheduler starts up it should read, it's saga log, and then reschedule non completed work. Startup returns a list of SagaIds, RecoverSagaState should then be called per SagaId. These methods are separate to allow the caller to determine how to handle failures (how many times to retry etc...) also to handle the rate at which the SagaLog is polled, and how quickly tasks are rescheduled. These decisions are specific to an implementation and therefore should not be abstracted from the user by the Saga package.

This code review also contains Saga Recovery Logic, which reads Sagas from the SagaLog, replays messages and verifies state is correct, and checks if its safe to proceed otherwise aborts the Saga.

The demo has been updated to poll the InMemorySagaLog (which just returns the ids of all sagas started since creation) after running all the work. It then verifies that all sagas are in the completed state.

* Returns all Sagas Started since this InMemory Saga was created
*/
func (log *inMemorySagaLog) GetActiveSagas() ([]string, error) {
log.mutex.RLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just defer the unlock after acquiring it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defer has a pretty high performance cost, given how simple this code is i just chose to explicitly place it

Copy link
Contributor

Choose a reason for hiding this comment

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

The cost is fairly low, if this isn't a hot spot I'd suggest we follow the idiom.

If this is a hot spot, that sounds like performance optimization. Does it make sense to use the syntax for performance optimizations we discussed?

Copy link
Contributor

Choose a reason for hiding this comment

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

(BTW, just googled an found this, golang/go#6980 , which shows they know about it, and the cost is ~70ns)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure just switched to lock, defer unlock since its just test code

@dbentley
Copy link
Contributor

LGTM

@codecov-io
Copy link

codecov-io commented Jun 16, 2016

Current coverage is 72.50%

Merging #27 into master will increase coverage by 3.63%

@@             master        #27   diff @@
==========================================
  Files            14         15     +1   
  Lines           559        611    +52   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            385        443    +58   
+ Misses          155        147     -8   
- Partials         19         21     +2   

Powered by Codecov. Last updated by 7fc0a9b...1afdd8c

@CaitieM20 CaitieM20 merged commit 986cc84 into master Jun 16, 2016
}

case ForwardRecovery:

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you omitting the TODO here because that would mean having to create a new ticket? Would it be ok to attribute this to an existing/task story?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is nothing really to do on ForwardRecovery, you are always in a safe state. I can add a comment, explaining this, since Tasks have to be Idempotent.

@dgassaway dgassaway deleted the caitie/recoverState2 branch June 8, 2019 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants