Skip to content

Update node list on workers#12094

Closed
dain wants to merge 4 commits intoprestodb:masterfrom
dain:poll-nodes-on-workers
Closed

Update node list on workers#12094
dain wants to merge 4 commits intoprestodb:masterfrom
dain:poll-nodes-on-workers

Conversation

@dain
Copy link
Contributor

@dain dain commented Dec 18, 2018

Some connectors require an active list of all worers on worker nodes. This
commit restores this behavior which was recently removed.

@kokosing
Copy link
Contributor

Is it possible to have test for that?

@dain
Copy link
Contributor Author

dain commented Dec 21, 2018

@kokosing and @electrum I added a test for the notification system, which didn't have tests before.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you use try-with-resources for manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it doesn't implement AutoCloseable

@dain dain force-pushed the poll-nodes-on-workers branch 4 times, most recently from f8aec11 to 4a0057c Compare January 7, 2019 19:47
dain added 4 commits January 7, 2019 11:51
Some connectors require an active list of all worers on worker nodes.  This
commit restores this behavior which was recently removed.
Instead of firing node change event each time nodes are polled, fire it only
when the node set changes.
@dain dain force-pushed the poll-nodes-on-workers branch from 4a0057c to c240f0d Compare January 7, 2019 19:51
@dain
Copy link
Contributor Author

dain commented Jan 7, 2019

@electrum and @kokosing I added a test, but in the process I need to add the commit "Only fire node change event on change". Can one of you review that commit and the test?

Copy link
Contributor

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

Can you also test that announcing same set of nodes (that were already announced) does not trigger any notification?

@electrum electrum self-assigned this Jan 9, 2019
@raghavsethi
Copy link
Contributor

Merged.

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.

5 participants