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

Unpin grpc version and use serviceConfig to set the load balancer #1786

Merged
merged 3 commits into from
Sep 12, 2019
Merged

Unpin grpc version and use serviceConfig to set the load balancer #1786

merged 3 commits into from
Sep 12, 2019

Conversation

guanw
Copy link
Contributor

@guanw guanw commented Sep 9, 2019

Signed-off-by: Jude Wang judew@uber.com

Which problem is this PR solving?

Short description of the changes

  • This PR aims to unpin grpc and replace deprecated code

Signed-off-by: Jude Wang <judew@uber.com>
@guanw
Copy link
Contributor Author

guanw commented Sep 9, 2019

I notice there's a new commit detected in git for idl/ and not sure if I should commit it.

@guanw
Copy link
Contributor Author

guanw commented Sep 10, 2019

@pavolloffay One question regarding the TestSpanReader_multiRead_followUp_query(introduced in #1677 ), after upgrading lock file this test starts to fail where mocks Index method was expecting two string instead of one. Any idea why?

@guanw
Copy link
Contributor Author

guanw commented Sep 10, 2019

@pavolloffay Actually it feels like TestSpanReader_multiRead_followUp_query is being flaky since the s.timeRangeIndices(s.spanIndexPrefix, startTime.Add(-time.Hour), endTime.Add(time.Hour)) could compute slice with either one entry or two based on the time zone and time.Now(). I think you need to add a second mock.AnythingOfType("string") to .On("Index", ...) mock function.

@pavolloffay
Copy link
Member

I notice there's a new commit detected in git for idl/ and not sure if I should commit it.

If it is not related to this change it should be reverted.

@pavolloffay
Copy link
Member

@pavolloffay One question regarding the TestSpanReader_multiRead_followUp_query(introduced in #1677 ), after upgrading lock file this test starts to fail where mocks Index method was expecting two string instead of one. Any idea why?

The changes here does not seem to be related to the ES reader. The test is probably flaky. Could oyu please report that in a separate issue?

Signed-off-by: Yuri Shkuro <ys@uber.com>
@yurishkuro
Copy link
Member

There is already an issue for that test: #1713

@codecov
Copy link

codecov bot commented Sep 11, 2019

Codecov Report

Merging #1786 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1786      +/-   ##
==========================================
- Coverage   98.27%   98.23%   -0.05%     
==========================================
  Files         195      195              
  Lines        9555     9555              
==========================================
- Hits         9390     9386       -4     
- Misses        131      134       +3     
- Partials       34       35       +1
Impacted Files Coverage Δ
pkg/discovery/grpcresolver/grpc_resolver.go 98.57% <ø> (ø) ⬆️
cmd/agent/app/reporter/grpc/builder.go 96.55% <100%> (ø) ⬆️
cmd/query/app/static_handler.go 83.33% <0%> (-3.51%) ⬇️

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 dbfeb7a...c254c04. Read the comment docs.

@yurishkuro yurishkuro changed the title (WIP) Unpin grpc version and use serviceConfig to set loadbalancer Unpin grpc version and use serviceConfig to set the load balancer Sep 12, 2019
@yurishkuro yurishkuro merged commit 3fa1dfa into jaegertracing:master Sep 12, 2019
@yurishkuro yurishkuro added this to the Release 1.15 milestone Nov 7, 2019
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.

3 participants