Skip to content

Support one handler for mulitple GCP services and fixes#1867

Merged
istio-merge-robot merged 2 commits intoistio:masterfrom
manlinl:int2
Nov 28, 2017
Merged

Support one handler for mulitple GCP services and fixes#1867
istio-merge-robot merged 2 commits intoistio:masterfrom
manlinl:int2

Conversation

@manlinl
Copy link
Copy Markdown
Contributor

@manlinl manlinl commented Nov 27, 2017

  • Allow one handler handles multiple GCP services
  • Expand metric support
  • Minor fixes in reportBuilder

@istio-testing
Copy link
Copy Markdown
Collaborator

@manlinl: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

Details

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 27, 2017

Codecov Report

Merging #1867 into master will increase coverage by 0.44%.
The diff coverage is 59.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1867      +/-   ##
==========================================
+ Coverage    81.9%   82.34%   +0.44%     
==========================================
  Files         188      200      +12     
  Lines       18715    19783    +1068     
==========================================
+ Hits        15328    16291     +963     
- Misses       2960     3053      +93     
- Partials      427      439      +12
Flag Coverage Δ
#broker 45.51% <ø> (+1.06%) ⬆️
#mixer 83.21% <59.67%> (+0.73%) ⬆️
#pilot 82.18% <ø> (-0.1%) ⬇️
#security 90.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
mixer/adapter/svcctrl/handler.go 35.13% <51.11%> (+11.32%) ⬆️
mixer/adapter/svcctrl/reportbuilder.go 87.5% <82.35%> (+3.62%) ⬆️
pilot/adapter/config/memory/monitor.go 79.48% <0%> (-10.26%) ⬇️
pilot/adapter/config/crd/controller.go 77.85% <0%> (-2.02%) ⬇️
mixer/pkg/runtime/handlerTable.go 96.62% <0%> (ø)
mixer/pkg/runtime/context.go 100% <0%> (ø)
mixer/pkg/runtime/logger.go 100% <0%> (ø)
mixer/pkg/runtime/monitor.go 100% <0%> (ø)
mixer/pkg/runtime/handler.go 89.92% <0%> (ø)
mixer/pkg/runtime/init.go 28.33% <0%> (ø)
... and 7 more

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 cc39692...1f60b23. Read the comment docs.

- Allow one handler handles multiple GCP services
- Expand metric support
- Minor fixes in reportBuilder
svcProc, err := h.getServiceProcessor(ctx)
if err != nil {
return adapter.QuotaResult{
Status: status.WithPermissionDenied(err.Error()),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this internal error? It does not look like a permission issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed


// lock protects svcProcMap.
lock sync.Mutex
svcProcMap map[string]*serviceProcessor
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please comment what's the key/value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done


func newHandler(ctx *handlerContext) (*handler, error) {
return &handler{
ctx: ctx,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please test this code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Copy Markdown
Contributor Author

@manlinl manlinl left a comment

Choose a reason for hiding this comment

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

PTAL


// lock protects svcProcMap.
lock sync.Mutex
svcProcMap map[string]*serviceProcessor
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

svcProc, err := h.getServiceProcessor(ctx)
if err != nil {
return adapter.QuotaResult{
Status: status.WithPermissionDenied(err.Error()),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed


func newHandler(ctx *handlerContext) (*handler, error) {
return &handler{
ctx: ctx,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

@apicl
Copy link
Copy Markdown
Contributor

apicl commented Nov 28, 2017

/lgtm

@apicl
Copy link
Copy Markdown
Contributor

apicl commented Nov 28, 2017

/approve

@istio-merge-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apicl

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@manlinl
Copy link
Copy Markdown
Contributor Author

manlinl commented Nov 28, 2017

/test istio-pilot-e2e

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot istio-merge-robot merged commit 9783939 into istio:master Nov 28, 2017
@manlinl manlinl deleted the int2 branch November 28, 2017 03:49
vadimeisenbergibm pushed a commit to vadimeisenbergibm/istio that referenced this pull request Nov 28, 2017
Automatic merge from submit-queue.

Support one handler for mulitple GCP services and fixes

- Allow one handler handles multiple GCP services
- Expand metric support
- Minor fixes in reportBuilder
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.

5 participants