Skip to content
This repository was archived by the owner on Apr 29, 2020. It is now read-only.

changed buffered channel#1034

Open
yzaccc wants to merge 1 commit intosquare:masterfrom
yzaccc:yangzongkun/p2-prepare-add-buffer-to-podChanMap-channel
Open

changed buffered channel#1034
yzaccc wants to merge 1 commit intosquare:masterfrom
yzaccc:yangzongkun/p2-prepare-add-buffer-to-podChanMap-channel

Conversation

@yzaccc
Copy link

@yzaccc yzaccc commented Mar 27, 2018

This commit is a complementary for this commit

Ensuring that the preparer is always using the latest pod manifest

go p.store.WatchPods(consul.INTENT_TREE, p.node, quitChan, errChan, podChan)

podChanMap := make(map[podWorkerID]chan ManifestPair)
podChanMap := make(map[podWorkerID]chan ManifestPair, 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think you actually want to buffer the chan ManifestPair on line 123

@@ -89,11 +89,11 @@ func (p *Preparer) WatchForPodManifestsForNode(quitAndAck chan struct{}) {
// buffer this with 1 manifest so that we can make sure that the
// consumer is always reading the latest manifest. Before writing to
// this channel, we will attempt to drain it first
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should move this comment to the place the actual buffering is

@yzaccc
Copy link
Author

yzaccc commented Mar 28, 2018

Disabled race testing for orchestrate test.

@@ -1,3 +1,5 @@
// +build !race
Copy link
Collaborator

Choose a reason for hiding this comment

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

what race did you hit? looks like this file doesn't depend on consulutil which has known races, so if the race detector showed a problem it's probably a real problem

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants