-
Notifications
You must be signed in to change notification settings - Fork 696
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
Watch EOR grpc method #2608
Watch EOR grpc method #2608
Conversation
@fujita what do you think? |
@fujita gentle ping. |
Sorry about the delay. why you don't use TableEvent? |
Thanks for replying. I mean EOR-method signals that all eor messages from peers are received. It corresponds with table and peers as well. |
I thought that simply EoR messages can be sent via TableEvent.Paths. But you prefer to know when a peer gets EoR messages in all families? If so, the proposal looks like reasonable. |
Actually, I like your idea to watch each eor message via Paths. It will be even more useful, will be come soon with implementation. |
2e034b4
to
6f7df43
Compare
I've tried to make it at least minimalistic as possible. |
@fujita What do you think about this PR? |
@@ -125,7 +125,7 @@ message WatchEventRequest { | |||
|
|||
message Table { | |||
message Filter { | |||
enum Type { BEST = 0; ADJIN = 1; POST_POLICY = 2; } | |||
enum Type { BEST = 0; ADJIN = 1; POST_POLICY = 2; EOR = 3; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? ADJIN is supposed to send all messages include EoR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? ADJIN is supposed to send all messages include EoR?
Adding a new filter provides opportunity of getting eor message itself without handling adj-in tables that can be huge.
I thought about adding it to Best actually, but it will probably not the right place (tell me if I'm wrong).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Is this backward compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks so. If you don't add a filter, you won't get an eor message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old binary (using Watch gRPC API) works with the gobgpd binary including this change?
If someone confirms that this change doesn't break the backward compatibility of the gRPC API, then I'll merge. |
Tested that: |
@mai2412 After reading the latest code, I guess that |
@fujita Ahh I see that the POST_POLICY filter has to be used, I did not see this before and only tried testing with BEST and ADJIN. It would be ideal if there's a filter for getting just the EOR message without the other updates. I think the original proposal of this PR that introduces a new message type EOR might also work well. What do you think? Do correct me if i said anything wrong, thank you for looking into this. |
Signed-off-by: Rinat Baygildin <[email protected]>
Signed-off-by: Rinat Baygildin <[email protected]>
At last I rebased the code with some minor changes.
|
@fujita gentle ping |
Pushed, thanks. |
Add new EOR type for WatchEvent method.
It provides opportunity of getting 'readiness' of the current peer.