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

Event deregister for Informer API #311

Merged
merged 3 commits into from Apr 28, 2020
Merged

Event deregister for Informer API #311

merged 3 commits into from Apr 28, 2020

Conversation

MSNTCS
Copy link
Contributor

@MSNTCS MSNTCS commented Apr 9, 2020

The subscription would be canceled if the client sends deregister or the
connection loses.

@MSNTCS MSNTCS requested a review from majecty April 9, 2020 08:18
@majecty
Copy link
Contributor

majecty commented Apr 9, 2020

@MSNTCS Could you write a test that uses register and deregister?

majecty
majecty requested changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@majecty majecty left a comment

Choose a reason for hiding this comment

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

Could you explain what happens to the ColdEventGenerator if a connection is deregistered?

informer/src/handler/rpc_ws_handler.rs Outdated Show resolved Hide resolved
informer/src/informer_service/informer_service_handler.rs Outdated Show resolved Hide resolved
informer/src/handler/rpc_ws_handler.rs Outdated Show resolved Hide resolved
@MSNTCS
Copy link
Contributor Author

MSNTCS commented Apr 9, 2020 •
edited

@MSNTCS Could you write a test that uses register and deregister?

Could you explain what happens to the ColdEventGenerator if a connection is deregistered?

Now, the connection would not be available but I am not sure about the thread of cold events. I will think about it.

@MSNTCS MSNTCS changed the title Event deregister for Informer API [WIP] Event deregister for Informer API Apr 9, 2020
@MSNTCS MSNTCS added the do-not-merge Do not merge this PR label Apr 10, 2020
@MSNTCS
Change Connection struct to Subscription struct
a703ade 
We do not handle web socket connections, Informer handles subscriptions
@MSNTCS MSNTCS changed the title [WIP] Event deregister for Informer API Event deregister for Informer API Apr 20, 2020
@MSNTCS MSNTCS requested a review from majecty April 20, 2020 09:34
majecty
majecty requested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@majecty majecty left a comment

Choose a reason for hiding this comment

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

What happens to the ColdEvent generators when a subscription is removed?

informer/src/handler/rpc_ws_handler.rs Outdated Show resolved Hide resolved
informer/src/informer_service/informer_service_handler.rs Outdated Show resolved Hide resolved
@MSNTCS MSNTCS force-pushed the master branch 4 times, most recently from 90bca50 to 86019b2 Compare April 24, 2020 04:24
majecty
majecty requested changes Apr 27, 2020
View reviewed changes
informer/src/handler/rpc_ws_handler.rs Outdated Show resolved Hide resolved
informer/src/handler/rpc_ws_handler.rs Outdated Show resolved Hide resolved
informer/src/informer_service/informer_service_handler.rs Outdated Show resolved Hide resolved
informer/src/handler/subscription.rs Outdated Show resolved Hide resolved
informer/src/handler/subscription.rs Outdated Show resolved Hide resolved
informer/src/handler/rpc_ws_handler.rs
Comment on lines +27 to +31
#[derive(Clone)]
pub enum Registration {
Register(Subscription),
Deregister(SubscriptionId),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

informer/src/informer_service/informer_service_handler.rs Outdated Show resolved Hide resolved
@MSNTCS MSNTCS force-pushed the master branch 2 times, most recently from cc206f8 to 32eb0ab Compare April 27, 2020 07:41
majecty
majecty requested changes Apr 27, 2020
View reviewed changes
informer/src/handler/subscription.rs Outdated Show resolved Hide resolved
informer/src/handler/subscription.rs Outdated Show resolved Hide resolved
informer/src/informer_service/informer_service_handler.rs Outdated Show resolved Hide resolved
informer/src/handler/subscription.rs Outdated Show resolved Hide resolved
informer/src/handler/subscription.rs Outdated Show resolved Hide resolved
informer/src/informer_service/informer_service_handler.rs Outdated Show resolved Hide resolved
@MSNTCS
Process the deregister request
60a146f 
The subscription would be canceled if the client sends deregister or the
connection loses
@MSNTCS MSNTCS requested a review from majecty April 27, 2020 08:13
@majecty
Copy link
Contributor

majecty commented Apr 27, 2020 •
edited

@MSNTCS Why do you set the do-not-merge label here? Isn't it ready to be merged?

@MSNTCS MSNTCS removed the do-not-merge Do not merge this PR label Apr 28, 2020
@MSNTCS
Copy link
Contributor Author

MSNTCS commented Apr 28, 2020

@MSNTCS Why do you set the do-not-merge label here? Isn't it ready to be merged?

@majecty forgot to remove it. Now, it is ready to be merged.

@majecty majecty merged commit abc2049 into CodeChain-io:master Apr 28, 2020
@MSNTCS MSNTCS mentioned this pull request Apr 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@majecty majecty majecty approved these changes

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
2 participants
@MSNTCS @majecty