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

Fix flaky tbuffered server test #2635

Merged

Conversation

pkositsyn
Copy link
Contributor

@pkositsyn pkositsyn commented Nov 20, 2020

Which problem is this PR solving?

Fixes #2631

Short description of the changes

The order for error and second valid packet is changed in fakeTransport. Now having second packet dropped guarantees that the error is already processed, which absence was the reason for flaky test

@pkositsyn pkositsyn requested a review from a team as a code owner November 20, 2020 00:17
Signed-off-by: Pavel Kositsyn <[email protected]>
@pkositsyn pkositsyn force-pushed the flaky_tbuffered_server_test branch from ba03eae to 3642e1b Compare November 20, 2020 00:17
@mergify mergify bot requested a review from jpkrohling November 20, 2020 00:18
@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #2635 (abd2e3c) into master (e114201) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2635      +/-   ##
==========================================
+ Coverage   95.08%   95.09%   +0.01%     
==========================================
  Files         213      213              
  Lines        9492     9491       -1     
==========================================
  Hits         9025     9025              
+ Misses        390      388       -2     
- Partials       77       78       +1     
Impacted Files Coverage Δ
plugin/storage/badger/spanstore/reader.go 95.37% <0.00%> (-0.72%) ⬇️
plugin/storage/badger/spanstore/writer.go 97.01% <0.00%> (-0.09%) ⬇️
plugin/storage/kafka/writer.go 100.00% <0.00%> (ø)
plugin/storage/integration/integration.go 77.90% <0.00%> (+0.55%) ⬆️
...lugin/sampling/strategystore/adaptive/processor.go 100.00% <0.00%> (+0.92%) ⬆️

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 e114201...abd2e3c. Read the comment docs.

@pkositsyn
Copy link
Contributor Author

@jpkrohling Could you review this please?

yurishkuro
yurishkuro previously approved these changes Nov 26, 2020
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

LGTM, with a few minor corrections to comments to make this more understandable (for me, at least)

cmd/agent/app/servers/tbuffered_server_test.go Outdated Show resolved Hide resolved
cmd/agent/app/servers/tbuffered_server_test.go Outdated Show resolved Hide resolved
@mergify mergify bot dismissed yurishkuro’s stale review November 27, 2020 00:12

Pull request has been modified.

yurishkuro
yurishkuro previously approved these changes Nov 27, 2020
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro
Copy link
Member

please do git commit -s -amend to sign the last commit

@pkositsyn
Copy link
Contributor Author

I tried to do that - but after I commited from github with incorrect name in profile had to merge into my local branch, so now it is third from the top broken commit...

@pkositsyn pkositsyn force-pushed the flaky_tbuffered_server_test branch from d9458ff to abd2e3c Compare November 27, 2020 00:28
@pkositsyn
Copy link
Contributor Author

Fixed with hard reset + amend

@mergify mergify bot dismissed yurishkuro’s stale review November 27, 2020 00:28

Pull request has been modified.

@yurishkuro yurishkuro merged commit 24fb6bc into jaegertracing:master Nov 27, 2020
@yurishkuro
Copy link
Member

Thanks!

@jpkrohling
Copy link
Contributor

Sorry for the delay in reviewing, and thanks @yurishkuro for picking it up.

yurishkuro added a commit that referenced this pull request Jan 13, 2021
…ectors (#2657)

* add metrics that show agent connection collector status

Signed-off-by: WalkerWang731 <[email protected]>

* update comment

Signed-off-by: WalkerWang731 <[email protected]>

* exec make fmt

Signed-off-by: WalkerWang731 <[email protected]>

* simplify function and add testing relevant code in the builder_test.go

Signed-off-by: WalkerWang731 <[email protected]>

* add comment in connect_metrics.go

Signed-off-by: WalkerWang731 <[email protected]>

* simplify code and changed use expvar to show target

Signed-off-by: WalkerWang731 <[email protected]>

* simplify code and changed use expvar to show target

Signed-off-by: WalkerWang731 <[email protected]>

* exec make fmt

Signed-off-by: WalkerWang731 <[email protected]>

* Fix collector panic due to sarama sdk returning nil error (#2654)

Signed-off-by: luhualin <[email protected]>

Co-authored-by: luhualin <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Fix flaky tbuffered server test (#2635)

* Fix flaky tbuffered server test

Signed-off-by: Pavel Kositsyn <[email protected]>

* Apply suggestions from code review - more readable comments

Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: Pavel Kositsyn <[email protected]>

Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Add github actions for integration tests (#2649)

* Add github action for jaeger integration tests

Signed-off-by: Ashmita Bohara <[email protected]>

* Create separate workflow for each integration test

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Clean-up GH action names (#2661)

Signed-off-by: WalkerWang731 <[email protected]>

* Fix for failures in badger integration tests (#2660)

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Add protogen validation test (#2662)

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Add github action for jaeger all-in-one image (#2663)

* Add github action for jaeger all-in-one image

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>

* Make steps self-explantory

Signed-off-by: Ashmita Bohara <[email protected]>

* Fix git tags issue

Signed-off-by: Ashmita Bohara <[email protected]>

* Fix ES integration test

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Update comment that looks confusing during builds

Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Use GitHub actions based build badges

Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Fix and minor improvements to all-in-one github action (#2667)

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Fix docker login issue with all-in-one build (#2668)

* Fix docker login issue with all-in-one build

Signed-off-by: Ashmita Bohara <[email protected]>

* Fix docker login issue with all-in-one build

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Fix issue with all-in-one build (#2669)

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Update cmd/agent/app/reporter/connect_metrics.go

accept suggestions

Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Update cmd/agent/app/reporter/connect_metrics.go

accept suggestions

Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* simplify the code that remove ConnectMetricsParams{} and integrate ConnectMetrics{}

Signed-off-by: WalkerWang731 <[email protected]>

* simplify the code that remove ConnectMetricsParams{} and integrate ConnectMetrics{}

Signed-off-by: WalkerWang731 <[email protected]>

* merage from the lastest master branch and exec make fmt

Signed-off-by: walker.wangxy <[email protected]>

* add comment on ConnectMetrics

Signed-off-by: WalkerWang731 <[email protected]>

* clear up redundant codes

Signed-off-by: WalkerWang731 <[email protected]>

Co-authored-by: WalkerWang731 <[email protected]>
Co-authored-by: Betula-L <[email protected]>
Co-authored-by: luhualin <[email protected]>
Co-authored-by: Pavel Kositsyn <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Co-authored-by: Ashmita <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Co-authored-by: walker.wangxy <[email protected]>
bhiravabhatla pushed a commit to bhiravabhatla/jaeger that referenced this pull request Jan 25, 2021
…ectors (jaegertracing#2657)

* add metrics that show agent connection collector status

Signed-off-by: WalkerWang731 <[email protected]>

* update comment

Signed-off-by: WalkerWang731 <[email protected]>

* exec make fmt

Signed-off-by: WalkerWang731 <[email protected]>

* simplify function and add testing relevant code in the builder_test.go

Signed-off-by: WalkerWang731 <[email protected]>

* add comment in connect_metrics.go

Signed-off-by: WalkerWang731 <[email protected]>

* simplify code and changed use expvar to show target

Signed-off-by: WalkerWang731 <[email protected]>

* simplify code and changed use expvar to show target

Signed-off-by: WalkerWang731 <[email protected]>

* exec make fmt

Signed-off-by: WalkerWang731 <[email protected]>

* Fix collector panic due to sarama sdk returning nil error (jaegertracing#2654)

Signed-off-by: luhualin <[email protected]>

Co-authored-by: luhualin <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Fix flaky tbuffered server test (jaegertracing#2635)

* Fix flaky tbuffered server test

Signed-off-by: Pavel Kositsyn <[email protected]>

* Apply suggestions from code review - more readable comments

Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: Pavel Kositsyn <[email protected]>

Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Add github actions for integration tests (jaegertracing#2649)

* Add github action for jaeger integration tests

Signed-off-by: Ashmita Bohara <[email protected]>

* Create separate workflow for each integration test

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Clean-up GH action names (jaegertracing#2661)

Signed-off-by: WalkerWang731 <[email protected]>

* Fix for failures in badger integration tests (jaegertracing#2660)

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Add protogen validation test (jaegertracing#2662)

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Add github action for jaeger all-in-one image (jaegertracing#2663)

* Add github action for jaeger all-in-one image

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <[email protected]>

* Make steps self-explantory

Signed-off-by: Ashmita Bohara <[email protected]>

* Fix git tags issue

Signed-off-by: Ashmita Bohara <[email protected]>

* Fix ES integration test

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Update comment that looks confusing during builds

Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Use GitHub actions based build badges

Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Fix and minor improvements to all-in-one github action (jaegertracing#2667)

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Fix docker login issue with all-in-one build (jaegertracing#2668)

* Fix docker login issue with all-in-one build

Signed-off-by: Ashmita Bohara <[email protected]>

* Fix docker login issue with all-in-one build

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Fix issue with all-in-one build (jaegertracing#2669)

Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Update cmd/agent/app/reporter/connect_metrics.go

accept suggestions

Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* Update cmd/agent/app/reporter/connect_metrics.go

accept suggestions

Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: WalkerWang731 <[email protected]>

* simplify the code that remove ConnectMetricsParams{} and integrate ConnectMetrics{}

Signed-off-by: WalkerWang731 <[email protected]>

* simplify the code that remove ConnectMetricsParams{} and integrate ConnectMetrics{}

Signed-off-by: WalkerWang731 <[email protected]>

* merage from the lastest master branch and exec make fmt

Signed-off-by: walker.wangxy <[email protected]>

* add comment on ConnectMetrics

Signed-off-by: WalkerWang731 <[email protected]>

* clear up redundant codes

Signed-off-by: WalkerWang731 <[email protected]>

Co-authored-by: WalkerWang731 <[email protected]>
Co-authored-by: Betula-L <[email protected]>
Co-authored-by: luhualin <[email protected]>
Co-authored-by: Pavel Kositsyn <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Co-authored-by: Ashmita <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Co-authored-by: walker.wangxy <[email protected]>
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.

Flaky test TestTBufferedServer_Metrics
3 participants