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

INDY-2263: Implement ViewChangeTriggerService and PrimaryConnectionMonitorService #1405

Merged
merged 39 commits into from
Dec 6, 2019
Merged

INDY-2263: Implement ViewChangeTriggerService and PrimaryConnectionMonitorService #1405

merged 39 commits into from
Dec 6, 2019

Conversation

skhoroshavin
Copy link
Member

No description provided.

Sergey Khoroshavin added 3 commits November 14, 2019 17:11
INDY-2263: Make ViewChangeService unsubscribe from messages on cleanup
80096cb 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
INDY-2263: Move instance change handling around
030255b 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
INDY-2263: Fix tests
ab9123f 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Merge remote-tracking branch 'upstream/master' into indy-2263
3c9b840 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Nov 20, 2019

This pull request introduces 5 alerts and fixes 1 when merging 3c9b840 into dfd3b09 - view on LGTM.com

new alerts:

  • 5 for Unused import

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 20, 2019

This pull request introduces 5 alerts and fixes 1 when merging de5926b into ddf15c2 - view on LGTM.com

new alerts:

  • 5 for Unused import

fixed alerts:

  • 1 for Unused import

Improve message validation performance a bit
35b7e47 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
ashcherbakov
ashcherbakov reviewed Nov 22, 2019
View reviewed changes
plenum/common/messages/internal_messages.py Outdated
# TODO: This is a kind of hack to make Node process NeedViewChange before replicas
# Possible solution to this (and some other hacks) would be adding separate node internal bus
# to which such messages are sent, processed, and then forwarded to replicas buses if needed
PreNeedViewChange = NamedTuple('PreNeedViewChange', [('view_no', int)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to rename NeedViewChange to StartViewChange and PreNeedViewChange to NeedViewChange?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. Probably better solution would be renaming PreNeedViewChange to NodeNeedViewChange (instead of PreNeedViewChange) and either keeping NeedViewChange as is or renaming it to ReplicaNeedViewChange. However introducing separate Node internal bus, from which messages are forwarded to Replicas internal buses looks like a clearer (but not cheaper) option for me, since it would not require introducing separate message at all. Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I would prefer to avoid having multiple messages doing the same, so a separate bus is a better solution I think. But if two messages are kept for a while, then I would prefer not to use 'PreViewChange...' name.

plenum/server/consensus/instance_change_service.py Outdated Show resolved Hide resolved
plenum/server/view_change/node_view_changer.py
from plenum.server.view_change.view_changer import ViewChanger, ViewChangerDataProvider
from storage.kv_store import KeyValueStorage


Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we just fully remove the class and move all the logic to ViewChangeTriggerService?
It looks like ViewChangeTriggerService needs to be responsible for

  • View change by freshness
  • Forced view changes
  • View Changes on primary disconnection
  • View Changes on master degradation

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it is much better be handled just by one service. Problem is that this logic is much more tangled with node code, so I decided to postpone it a bit until I resolve problems with main part - sending and handling InstanceChange messages themselves.

plenum/test/consensus/view_change/test_sim_view_change.py Show resolved Hide resolved
Sergey Khoroshavin added 2 commits November 22, 2019 17:49
INDY-2263: Fix some tests
ad49e50 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
INDY-2263: Fix unused imports
7d256b1 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Nov 22, 2019

This pull request introduces 5 alerts and fixes 1 when merging ad49e50 into cadb30d - view on LGTM.com

new alerts:

  • 5 for Unused import

fixed alerts:

  • 1 for Unused import

INDY-2263: Some renames to make code look more consistent
9c2212c 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Nov 22, 2019

This pull request fixes 1 alert when merging 9c2212c into cadb30d - view on LGTM.com

fixed alerts:

  • 1 for Unused import

Sergey Khoroshavin added 2 commits November 22, 2019 19:47
INDY-2263: Fix some tests
02b82f4 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
INDY-2263: Fix tests
5ed6fcf 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@skhoroshavin skhoroshavin changed the title [WIP] INDY-2263: Implement InstanceChangeService [WIP] INDY-2263: Implement ViewChangeTriggerService Nov 23, 2019
@lgtm-com
Copy link

lgtm-com bot commented Nov 23, 2019

This pull request introduces 1 alert and fixes 1 when merging 5ed6fcf into cadb30d - view on LGTM.com

new alerts:

  • 1 for __init__ method calls overridden method

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 25, 2019

This pull request introduces 1 alert and fixes 1 when merging 2c8071f into cadb30d - view on LGTM.com

new alerts:

  • 1 for __init__ method calls overridden method

fixed alerts:

  • 1 for Unused import

INDY-2263: Fix tests
33428be 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Nov 25, 2019

This pull request introduces 1 alert and fixes 1 when merging 33428be into cadb30d - view on LGTM.com

new alerts:

  • 1 for __init__ method calls overridden method

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 26, 2019

This pull request introduces 1 alert and fixes 1 when merging 689e790 into e652e80 - view on LGTM.com

new alerts:

  • 1 for __init__ method calls overridden method

fixed alerts:

  • 1 for Unused import

Sergey Khoroshavin added 2 commits November 26, 2019 19:39
Improve performance of simulation tests
7b9a0a4 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
INDY-2263: Fix tests
55f0d0e 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2019

This pull request introduces 1 alert and fixes 2 when merging 74630d6 into 2558cf9 - view on LGTM.com

new alerts:

  • 1 for __init__ method calls overridden method

fixed alerts:

  • 2 for Unused import

INDY-2263: Extract primary disconnection reaction logic
eb8deea 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2019

This pull request introduces 3 alerts and fixes 2 when merging eb8deea into 2558cf9 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import
  • 1 for __init__ method calls overridden method

fixed alerts:

  • 2 for Unused import

INDY-2263: Make flake8 happy
8099e28 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2019

This pull request introduces 1 alert and fixes 2 when merging 8099e28 into 2558cf9 - view on LGTM.com

new alerts:

  • 1 for __init__ method calls overridden method

fixed alerts:

  • 2 for Unused import

Sergey Khoroshavin added 4 commits December 3, 2019 12:55
INDY-2263: Revert some of performance optimizations
0769977 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
INDY-2263: Fix more tests
85c3e8f 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Merge remote-tracking branch 'upstream/master' into indy-2263
e09f6ca 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
INDY-2263: Fix tests after merge
8ae470d 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 3, 2019

This pull request introduces 1 alert and fixes 3 when merging 8ae470d into ece14e9 - view on LGTM.com

new alerts:

  • 1 for __init__ method calls overridden method

fixed alerts:

  • 3 for Unused import

INDY-2263: Fix more tests
c61176b 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 3, 2019

This pull request introduces 1 alert and fixes 3 when merging c61176b into ece14e9 - view on LGTM.com

new alerts:

  • 1 for __init__ method calls overridden method

fixed alerts:

  • 3 for Unused import

Merge remote-tracking branch 'upstream/master' into indy-2263
bc0ada4 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 4, 2019

This pull request introduces 1 alert and fixes 3 when merging bc0ada4 into 84375f8 - view on LGTM.com

new alerts:

  • 1 for __init__ method calls overridden method

fixed alerts:

  • 3 for Unused import

INDY-2263: Fix more tests
6122c74 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 4, 2019

This pull request introduces 1 alert and fixes 3 when merging 6122c74 into 84375f8 - view on LGTM.com

new alerts:

  • 1 for __init__ method calls overridden method

fixed alerts:

  • 3 for Unused import

@skhoroshavin skhoroshavin changed the title [WIP] INDY-2263: Implement ViewChangeTriggerService INDY-2263: Implement ViewChangeTriggerService and PrimaryConnectionMonitorService Dec 4, 2019
INDY-2263: Fix more tests
49c1bfc 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 4, 2019

This pull request introduces 2 alerts and fixes 3 when merging 49c1bfc into 84375f8 - view on LGTM.com

new alerts:

  • 2 for __init__ method calls overridden method

fixed alerts:

  • 3 for Unused import

INDY-2263: Fix more tests
ecbbf43 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 4, 2019

This pull request introduces 2 alerts and fixes 3 when merging ecbbf43 into 956cc45 - view on LGTM.com

new alerts:

  • 2 for __init__ method calls overridden method

fixed alerts:

  • 3 for Unused import

Sergey Khoroshavin added 2 commits December 5, 2019 14:54
INDY-2263: Fix more tests
b0e8f2b 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
INDY-2263: Fix more tests
7361b53 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 5, 2019

This pull request introduces 2 alerts and fixes 3 when merging 7361b53 into 956cc45 - view on LGTM.com

new alerts:

  • 2 for __init__ method calls overridden method

fixed alerts:

  • 3 for Unused import

INDY-2263: Fix more tests
7834bd1 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 5, 2019

This pull request introduces 2 alerts and fixes 3 when merging 7834bd1 into 956cc45 - view on LGTM.com

new alerts:

  • 2 for __init__ method calls overridden method

fixed alerts:

  • 3 for Unused import

INDY-2263: Fix more tests
8a21460 
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 5, 2019

This pull request introduces 2 alerts and fixes 3 when merging 8a21460 into 956cc45 - view on LGTM.com

new alerts:

  • 2 for __init__ method calls overridden method

fixed alerts:

  • 3 for Unused import

@skhoroshavin skhoroshavin merged commit 0a94829 into hyperledger:master Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashcherbakov ashcherbakov ashcherbakov left review comments

@Toktar Toktar Toktar approved these changes

@lampkin-diet lampkin-diet Awaiting requested review from lampkin-diet

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@skhoroshavin @Toktar @ashcherbakov