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.

Sign up for GitHub

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

Jump to bottom

Enhanced end condition #89

Merged
merged 15 commits into from
Aug 7, 2020
Merged

Enhanced end condition #89

merged 15 commits into from
Aug 7, 2020

Conversation

yfl92
Copy link

@yfl92 yfl92 commented Aug 7, 2020 •
edited by patrick-ogrady
Loading

Closes: #66

Add end condition end_at_tip and end_duration for the check:data command.
User can specify the end conditions under the data/end_conditions section of the configuration file.

For example, to exit when tip is reached OR after 3600 secs:

"data": {
  "end_conditions": {
    "end_at_tip": true,
    "end_duration": 3600
  }
}

Changes

  • Add end condition end_at_tip implementation
  • Add end condition end_duration implementation

@coveralls
Copy link

coveralls commented Aug 7, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 3236

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 71.687%
Totals Coverage Status
Change from base Build 3190: 0.0%
Covered Lines: 1585
Relevant Lines: 2211
💛 - Coveralls

@yfl92 yfl92 changed the title Luke/enhance end condition Enhance end condition Aug 7, 2020
patrick-ogrady
patrick-ogrady reviewed Aug 7, 2020
View reviewed changes
internal/statefulsyncer/stateful_syncer.go Outdated
tipDelay int64,
interval time.Duration,
) {
tc := time.NewTicker(interval)
Copy link
Contributor

Choose a reason for hiding this comment

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

I had no idea you could do this!! Very very cool!

patrick-ogrady
patrick-ogrady reviewed Aug 7, 2020
View reviewed changes
internal/tester/data.go Outdated
@@ -292,6 +298,31 @@ func (t *DataTester) StartPeriodicLogger(
return ctx.Err()
}

// WatchEndCondition starts go routines to watch the end conditions
func (t *DataTester) WatchEndCondition(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: WatchEndConditions

patrick-ogrady
patrick-ogrady reviewed Aug 7, 2020
View reviewed changes
configuration/configuration.go Outdated
@@ -311,6 +311,13 @@ type DataConfiguration struct {
// useful to just try to fetch all blocks before checking for balance
// consistency.
BalanceTrackingDisabled bool `json:"balance_tracking_disabled"`

// EndAtTip is an end condition. syncing will stop once tip is reached
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we nest these in an EndConditions object? I think we will have many end conditions?

patrick-ogrady
patrick-ogrady reviewed Aug 7, 2020
View reviewed changes
internal/statefulsyncer/stateful_syncer.go Outdated
}

if atTip {
log.Println("Node has reached tip")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: syncer has reached tip

patrick-ogrady
patrick-ogrady reviewed Aug 7, 2020
View reviewed changes
internal/statefulsyncer/stateful_syncer.go Outdated
atTip, err := s.blockStorage.AtTip(ctx, tipDelay)
if err != nil {
log.Printf(
"%s: unable to evaluate if node is at tip",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unable to evaluate if syncer is at tip

patrick-ogrady
patrick-ogrady reviewed Aug 7, 2020
View reviewed changes
internal/statefulsyncer/stateful_syncer.go
@@ -155,3 +157,60 @@ func (s *StatefulSyncer) BlockRemoved(

return err
}

// EndAtTipLoop runs a loop that evaluates end condition EndAtTip
func (s *StatefulSyncer) EndAtTipLoop(
Copy link
Contributor

Choose a reason for hiding this comment

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

@slowboat0 How do you feel about putting all end condition checks in a single go routine?

Copy link
Contributor

Choose a reason for hiding this comment

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

When I initially thought about this PR, I thought about doing it that way. However, there is a cleanliness in using separate goroutines.

Copy link
Author

@yfl92 yfl92 Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'd like to keep it this way. One caveat of using single goroutine is that one expensive check on the condition (http timeout, db query timeout..) can block other condition checking as well.

I'm open to refactor once we've added more end conditions.

patrick-ogrady
patrick-ogrady reviewed Aug 7, 2020
View reviewed changes
internal/statefulsyncer/stateful_syncer.go Outdated
func (s *StatefulSyncer) EndAtTipLoop(
ctx context.Context,
tipDelay int64,
interval time.Duration,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have this as an arg? This seems like it should be a const somewhere?

Copy link
Author

Choose a reason for hiding this comment

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

It is a constant used by caller https://github.com/coinbase/rosetta-cli/blob/luke/enhance-end-condition/internal/tester/data.go#L58-L62

@yfl92 yfl92 force-pushed the luke/enhance-end-condition branch from 59ffb55 to d1761c3 Compare August 7, 2020 20:41
patrick-ogrady
patrick-ogrady approved these changes Aug 7, 2020
View reviewed changes
Copy link
Contributor

@patrick-ogrady patrick-ogrady left a comment

Choose a reason for hiding this comment

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

We may need to rethink this a little to handle Construction API end conditions well, but this is a great start! 🎉

@patrick-ogrady patrick-ogrady changed the title Enhance end condition Enhanced end condition Aug 7, 2020
@patrick-ogrady patrick-ogrady merged commit 5c7be42 into master Aug 7, 2020
@patrick-ogrady patrick-ogrady deleted the luke/enhance-end-condition branch August 7, 2020 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrick-ogrady patrick-ogrady patrick-ogrady approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhanced End Conditions
4 participants
@yfl92 @coveralls @patrick-ogrady @lzdl-cb