Nonblocking events #154
Nonblocking events #154
Conversation
There was a problem hiding this comment.
Good idea to cleanup tests. We will need to extract events queue before we move to SSE. I left some comments. I think we should split this PR into more commits or multiple PRs. One per bullet point you mentioned:
- make format and make check that runs same metalinter command as travis.ci
- bumped Consul version to 0.7.2 to fix random segfaults on Mac
- fixed race condition detected by go test -race in MarathonerStub
- fixed race condition in even_hander_test that caused tests to fail randomly (2nd commit)
- Drop events when queue is full
| @@ -27,6 +29,13 @@ test: deps $(SOURCES) | |||
| PATH=$(CURRENT_DIR)/bin:$(PATH) go test $(PACKAGES) $(TESTARGS) | |||
| go vet $(PACKAGES) | |||
|
|
|||
| gometalint-exists: | |||
| @which gometalinter > /dev/null || \ | |||
There was a problem hiding this comment.
We can fallback with gometalinter installation go get -u github.com/alecthomas/gometalinter
There was a problem hiding this comment.
good idea, but i would rather not have makefile installing dependencies when running build like this. instead we can create new target like install-tools which would do this job, and error message in -exists tasks would point to running make install-tools. So i will leave this as it is for now and we can introduce this additional target along with changing travis.yaml to use Make
There was a problem hiding this comment.
Agree. You can add it to deps target
| @@ -59,4 +68,7 @@ bump: | |||
| git add .goxc.json | |||
| git commit -m "Bumped version" | |||
|
|
|||
| format: | |||
| go fmt $(PACKAGES) | |||
| "github.com/allegro/marathon-consul/apps" | ||
| "github.com/allegro/marathon-consul/service" | ||
| consulapi "github.com/hashicorp/consul/api" | ||
| "sync" |
There was a problem hiding this comment.
We use goimports to group imports. Please move "sync" up.
| TasksStub map[apps.AppID][]*apps.Task | ||
| leader string | ||
| interactionsMu sync.RWMutex | ||
| interactions bool |
There was a problem hiding this comment.
How about changing interactions to int and count them. Then we can use atomic functions on it and we do not need mutex.
There was a problem hiding this comment.
When using atomic struct containing such field must be properly aligned: golang/go#13868
There was a problem hiding this comment.
I don't think it's our case. We don't target to ARM (we can drop support for x86) and we only need uint32
There was a problem hiding this comment.
I would rather leave it with mutex for simplicity/readability sake - no need for "smart" code here, this is just test stub (not hot part of code) and from what i see bool is enough. We can add counter when/If we need it.
| // then | ||
| assertDropped(t, recorder) | ||
| // stopChan returned by newEventHandler can be used as a synchronization | ||
| // point to wait until previous event has been processed |
There was a problem hiding this comment.
This comment should be converted to function comment
| @@ -9,6 +9,7 @@ import ( | |||
| "github.com/allegro/marathon-consul/events" | |||
| "github.com/allegro/marathon-consul/metrics" | |||
|
|
|||
| "errors" | |||
make check runs gometalinter with the same configuration as in CI builds. make format runs goimports on all project files.
web_handler tests have been moved to separate file and no longer use event_handler, only queue interactions are checked. event_handler tests no longer spin web_handler, instead tested events are pushed directly to event queue.
adamdubiel
force-pushed
the
nonblocking_events
branch
from
3607a6b to
9933ee6
Compare
|
@janisz i added requested changes: split change into commits and |
In addition to adding nonblocking handling of events i added:
make formatandmake checkthat runs same metalinter command as travis.cigo test -raceinMarathonerStubeven_hander_testthat caused tests to fail randomly (2nd commit)Bit more about test fixes. I moved
web_handlertest suite toweb_handler_testand it tests onlyweb_handler, the event queue is stubbed.event_handlertests on the other hand no longer startweb_handler, instead events that should be processed are sent directly to the event queue. This fixes a non-trivial race condition in tests, where success of test depended on wether request will be processed byweb_handlerand sent to the queue before test reaches synchronisation point at blocking send toevent_handlerstopChan.