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

Watch EOR grpc method #2608

Closed
wants to merge 2 commits into from
Closed

Watch EOR grpc method #2608

wants to merge 2 commits into from

Conversation

bayrinat
Copy link
Contributor

@bayrinat bayrinat commented Dec 13, 2022

Add new EOR type for WatchEvent method.
It provides opportunity of getting 'readiness' of the current peer.

@bayrinat bayrinat changed the title Watch EOR go grpc method Watch EOR grpc method Dec 13, 2022
@bayrinat
Copy link
Contributor Author

@fujita what do you think?

@bayrinat
Copy link
Contributor Author

@fujita gentle ping.

@fujita
Copy link
Member

fujita commented Dec 19, 2022

Sorry about the delay. why you don't use TableEvent?

@bayrinat
Copy link
Contributor Author

bayrinat commented Dec 19, 2022

Sorry about the delay. why you don't use TableEvent?

Thanks for replying.
Could you please explain the reasons? Is the TableEvent a decent type?

I mean EOR-method signals that all eor messages from peers are received. It corresponds with table and peers as well.
If I add it to TableEvent or to PeerEvent, it will be confusing a little bit.

@fujita
Copy link
Member

fujita commented Dec 21, 2022

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.

@bayrinat
Copy link
Contributor Author

Actually, I like your idea to watch each eor message via Paths. It will be even more useful, will be come soon with implementation.

@bayrinat bayrinat force-pushed the watch-eor branch 3 times, most recently from 2e034b4 to 6f7df43 Compare December 21, 2022 16:37
@bayrinat
Copy link
Contributor Author

I've tried to make it at least minimalistic as possible.

@bayrinat
Copy link
Contributor Author

@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; }
Copy link
Member

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?

Copy link
Contributor Author

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).

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

@mai2412
Copy link

mai2412 commented Jan 9, 2024

I would really like this feature as well (unless there was another way of watching EOR). Would you recommend opening a new pr or is it possible to merge this? @fujita @bayrinat

@fujita
Copy link
Member

fujita commented Jan 10, 2024

If someone confirms that this change doesn't break the backward compatibility of the gRPC API, then I'll merge.

@mai2412
Copy link

mai2412 commented Jan 10, 2024

Tested that:
$ gobgp monitor global rib / adj-in / neighbor
works with the gobgpd built from this change.

@fujita
Copy link
Member

fujita commented Jan 11, 2024

@mai2412 After reading the latest code, I guess that watchEventUpdate enables you to monitor EOR.

@mai2412
Copy link

mai2412 commented Jan 11, 2024

@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.

@bayrinat
Copy link
Contributor Author

At last I rebased the code with some minor changes.

  • This change still doesn't break the backward compatibility and works stable.

@bayrinat
Copy link
Contributor Author

bayrinat commented Mar 1, 2024

@fujita gentle ping

@fujita
Copy link
Member

fujita commented Mar 2, 2024

Pushed, thanks.

@fujita fujita closed this Mar 2, 2024
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.

3 participants