-
Notifications
You must be signed in to change notification settings - Fork 129
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
[CHANGED] Travis build changes #680
Conversation
- Use Ubuntu focal 20.04 - Added testing against nats-server latest release, and against the `dev` branch - Changed gcc `sanitize=address` builds to Debug to get line numbers in the output - Added `NATS_TEST_VALGRIND=yes` to `sanitize=thread` to reduce repeat counts and avoid timeouts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestion otherwise LGTM
Co-authored-by: Ziya Suzen <ziya@suzen.net>
test/test.c
Outdated
@@ -16311,6 +16311,11 @@ test_ServerPoolUpdatedOnClusterUpdate(void) | |||
const char *urls[] = {"127.0.0.1:4222", "127.0.0.1:4223", "127.0.0.1:4224"}; | |||
test("Check pool: "); | |||
s = _checkPool(conn, (char**)urls, (int)(sizeof(urls)/sizeof(char*))); | |||
if (s != NATS_OK) | |||
{ | |||
nats_Sleep(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kozlovic if you don't mind a quick look here to confirm.
I think the the (connected) server was sending 2 INFO packets, the first was coming with obsolete list of URL, and then the second one would correct it. Not sure if there's a better way to wait for the second info packet to arrive in the test, but this seems to have fixed the flapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not able to reproduce, but _checkPool() is already sleeping and waiting until it gets the expected array, but I realize that we don't retry for the expected size (just the expected content). We could improve _checkPool instead of adding the sleep here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could still maintain the error message by moving it at the end of function. But LGTM.
@@ -7430,14 +7430,15 @@ _checkPool(natsConnection *nc, char **expectedURLs, int expectedURLsCount) | |||
bool ok; | |||
|
|||
natsMutex_Lock(nc->mu); | |||
if (nc->srvPool->size != expectedURLsCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could still have this "if/print error" statement at the end (before releasing the mutex) so that we have this printed in case it helps.
...
if (nc->srvPool->size != expectedURLsCount)
printf("Expected pool size to be %d, got %d\n", expectedURLsCount, nc->srvPool->size);
natsMutex_Unlock(nc->mu);
return NATS_ERR;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kozlovic this seemed like a 1-liner to add, but upon further reading the function, I don't think the retry logic was quite correct, so I refactored the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* [CHANGED] Travis build changes (#680) * [CHANGED] Travis build changes - Use Ubuntu focal 20.04 - Added testing against nats-server latest release, and against the `dev` branch - Changed gcc `sanitize=address` builds to Debug to get line numbers in the output - Added `NATS_TEST_VALGRIND=yes` to `sanitize=thread` to reduce repeat counts and avoid timeouts * Update buildOnTravis.sh Co-authored-by: Ziya Suzen <ziya@suzen.net> * Fixed flapping test_ServerPoolUpdatedOnClusterUpdate * PR feedback: test _checkPool to retry # subjects * PR feedback: refactored _checkPool --------- Co-authored-by: Ziya Suzen <ziya@suzen.net> * [ADDED] Support for (multiple) ConsumerConfig.FilterSubjects (#679) * [CHANGED] Travis build changes - Use Ubuntu focal 20.04 - Added testing against nats-server latest release, and against the `dev` branch - Changed gcc `sanitize=address` builds to Debug to get line numbers in the output - Added `NATS_TEST_VALGRIND=yes` to `sanitize=thread` to reduce repeat counts and avoid timeouts * Update buildOnTravis.sh Co-authored-by: Ziya Suzen <ziya@suzen.net> * Fixed flapping test_ServerPoolUpdatedOnClusterUpdate * [ADDED] Support for (multiple) ConsumerConfig.FilterSubjects * PR feedback: took out a redundant check * point travis to build with NATS main, not dev --------- Co-authored-by: Ziya Suzen <ziya@suzen.net> * [Added] More v2.10 related changes (#682) * Added Metadata to Stream, Consumer configs Merged some other fixes from go PR * leak * Added jsStreamConfig.Compression * Added jsStreamConfig.FirstSeq * Added jsStreamConfig.SubjectTransform * Added jsStreamSourceInfo.SubjectTransforms and .FilterSubjects * Added jsStreamConfig.ConsumerLimits * PR feedback: nits * PR feedback: verify metadata values in test * PR feedback: fix a cast * Fixed the broken test (order of values) --------- Co-authored-by: Ziya Suzen <ziya@suzen.net>
This PR prepares for testing with nats server v2.10 changes. It also fixes an issue with
sanitize=thread
that seemed to have appeared due to Travisbionic
image changes.focal
v20.04latest
release, and against thedev
branchsanitize=address
builds to Debug to get line numbers in the outputNATS_TEST_VALGRIND=yes
tosanitize=thread
to reduce repeat counts and avoid timeouts