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

INDY-2223: ViewChange message serialization #1338

Merged

Conversation

Toktar
Copy link
Contributor

@Toktar Toktar commented Sep 18, 2019

Signed-off-by: toktar renata.toktar@dsr-corporation.com

Sergey Khoroshavin and others added 4 commits September 17, 2019 11:27
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sergey Khoroshavin and others added 4 commits September 18, 2019 21:43
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 19, 2019

This pull request introduces 3 alerts when merging fccfaf8 into 018526a - view on LGTM.com

new alerts:

  • 2 for __eq__ not overridden when adding attributes
  • 1 for Unused import

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 19, 2019

This pull request introduces 3 alerts when merging 3fdf4d9 into 018526a - view on LGTM.com

new alerts:

  • 2 for __eq__ not overridden when adding attributes
  • 1 for Unused import

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 19, 2019

This pull request introduces 4 alerts when merging 621e6b7 into 018526a - view on LGTM.com

new alerts:

  • 2 for __eq__ not overridden when adding attributes
  • 2 for Unused import

@skhoroshavin
Copy link
Member

test this please

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
@Toktar Toktar changed the title [WIP][ci skip][INDY-2223] ViewChange message serialization INDY-2223: ViewChange message serialization Sep 19, 2019
@lgtm-com
Copy link

lgtm-com bot commented Sep 19, 2019

This pull request introduces 4 alerts when merging 987e118 into 018526a - view on LGTM.com

new alerts:

  • 2 for __eq__ not overridden when adding attributes
  • 2 for Unused import

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 19, 2019

This pull request introduces 4 alerts when merging fac6419 into f68d274 - view on LGTM.com

new alerts:

  • 2 for __eq__ not overridden when adding attributes
  • 2 for Unused import

skhoroshavin
skhoroshavin previously approved these changes Sep 19, 2019
Copy link
Member

@skhoroshavin skhoroshavin left a comment

Choose a reason for hiding this comment

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

I've left some comments that need to be addressed, but this should be done in next PR - this one is both good and urgent enough to be merged now

(NonNegativeNumberField().validate, pp_view_no),
(NonNegativeNumberField().validate, pp_seq_no),
(NonEmptyStringField().validate, pp_digest)):
bid = BatchID(*val)
Copy link
Member

Choose a reason for hiding this comment

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

  1. This line can throw, and as far as I understand caller of _specific_validation doesn't expect exceptions. If so it needs to be fixed to just return an error (either by wrapping in try/except or adding additional checks)
  2. Serialized BatchID should be dictionary, not list.


# An example when `view_no` != `pp_view_no`, is when view change didn't finish at first round
# (next primary is unavailable for example)
from typing import NamedTuple
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather move import to the top, and keep comments attached to BatchID definition

serialized_msg = Batched().prepForSending(msg)
serialized_msg = ZStack.serializeMsg(serialized_msg)
new_msg = node_message_factory.get_instance(**ZStack.deserializeMsg(serialized_msg))
if not isinstance(msg, MessageRep):
Copy link
Member

Choose a reason for hiding this comment

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

Why exclude MessageRep here?

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 19, 2019

This pull request introduces 4 alerts when merging 2685376 into 0032d6b - view on LGTM.com

new alerts:

  • 2 for __eq__ not overridden when adding attributes
  • 2 for Unused import

@skhoroshavin skhoroshavin merged commit 195e61a into hyperledger:master Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants