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.

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

[tests] Increase timeout for TestGRPCResolverRoundRobin #2729

Merged
merged 4 commits into from
Jan 14, 2021

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Jan 14, 2021

Resolves #2025.

At first this was just adding extra logging, but per the next comment the logging indicated that it's indeed a timing issue.

Also did some clean-up of the code, like pre-allocating arrays with the right sizes.

Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro requested a review from a team as a code owner January 14, 2021 00:41
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro
Copy link
Member Author

ha, interesting, the logging already yielded fruit, the test failed like this:

    grpc_resolver_test.go:100: 
        	Error Trace:	grpc_resolver_test.go:100
        	            				grpc_resolver_test.go:156
        	Error:      	Should be true
        	Test:       	TestGRPCResolverRoundRobin/minPeers=3
        	Messages:   	Connection #0 was still not up. Connections so far: map[]

Because it's the very first connection that failed, it can only be due to timing, as there are no other conditions. Also, first connections in the subsequent tests are often acquired after 2-3 hundred iterations, almost half of the total time, e.g. this one at 400:

=== RUN   TestGRPCResolverRoundRobin/minPeers=7
    grpc_resolver_test.go:95: connected to peer #0 (127.0.0.1:40041) on iteration 440
    grpc_resolver_test.go:95: connected to peer #1 (127.0.0.1:44157) on iteration 1
    grpc_resolver_test.go:95: connected to peer #2 (127.0.0.1:41503) on iteration 0
    grpc_resolver_test.go:95: connected to peer #3 (127.0.0.1:38727) on iteration 1
    grpc_resolver_test.go:95: connected to peer #4 (127.0.0.1:32919) on iteration 0

Even though we have a pretty long timeout already (1000 * 10ms = 10s), I am going to increase it even more.

@codecov
Copy link

codecov bot commented Jan 14, 2021

Codecov Report

Merging #2729 (dface2f) into master (2f8ffa9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2729      +/-   ##
==========================================
+ Coverage   95.73%   95.75%   +0.01%     
==========================================
  Files         216      217       +1     
  Lines        9599     9619      +20     
==========================================
+ Hits         9190     9211      +21     
  Misses        336      336              
+ Partials       73       72       -1     
Impacted Files Coverage Δ
cmd/agent/app/reporter/connect_metrics.go 100.00% <100.00%> (ø)
cmd/agent/app/reporter/grpc/builder.go 95.83% <100.00%> (+0.83%) ⬆️
cmd/agent/app/reporter/grpc/collector_proxy.go 100.00% <100.00%> (ø)
pkg/discovery/grpcresolver/grpc_resolver.go 100.00% <100.00%> (ø)
plugin/storage/integration/integration.go 77.34% <0.00%> (-0.56%) ⬇️
cmd/query/app/static_handler.go 96.52% <0.00%> (+1.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 893c837...dface2f. Read the comment docs.

@yurishkuro yurishkuro changed the title [tests] Add logging for TestGRPCResolverRoundRobin [tests] Increase timeout for TestGRPCResolverRoundRobin Jan 14, 2021
@@ -84,19 +84,20 @@ func makeSureConnectionsUp(t *testing.T, count int, testc grpctest.TestServiceCl
// Make sure connections to all servers are up.
for si := 0; si < count; si++ {
connected := false
for i := 0; i < 1000; i++ {
for i := 0; i < 3000; i++ { // 3000 * 10ms = 30s
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the main change/fix

@yurishkuro yurishkuro merged commit 2ffdaae into jaegertracing:master Jan 14, 2021
@yurishkuro yurishkuro deleted the fix-2085 branch January 14, 2021 01:26
bhiravabhatla pushed a commit to bhiravabhatla/jaeger that referenced this pull request Jan 25, 2021
…g#2729)

* [tests] Add logging for TestGRPCResolverRoundRobin

Signed-off-by: Yuri Shkuro <github@ysh.us>

* rename var for clarity

Signed-off-by: Yuri Shkuro <github@ysh.us>

* cleanup

Signed-off-by: Yuri Shkuro <github@ysh.us>

* increase timeout

Signed-off-by: Yuri Shkuro <github@ysh.us>
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.

None yet

1 participant