Skip to content

Agg watcher reduce usage pr#787

Merged
centaurus-cloud-bot merged 3 commits intoCentaurusInfra:masterfrom
Sindica:aggWatcher-reduce-usage-PR
Oct 23, 2020
Merged

Agg watcher reduce usage pr#787
centaurus-cloud-bot merged 3 commits intoCentaurusInfra:masterfrom
Sindica:aggWatcher-reduce-usage-PR

Conversation

@Sindica
Copy link
Collaborator

@Sindica Sindica commented Oct 20, 2020

Perf run in https://github.com/futurewei-cloud/arktos-perftest/tree/perf-20201019
CI passed in #784
Compacted commits for easiler review

@Sindica
Copy link
Collaborator Author

Sindica commented Oct 20, 2020

/assign yb01

@centaurus-cloud-bot
Copy link
Collaborator

@Sindica: GitHub didn't allow me to assign the following users: yb01.

Note that only centaurus-cloud members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign yb01

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.

@Sindica Sindica requested a review from yb01 October 21, 2020 15:48
return false, 0
}

err := aggWatchers.GetErrors()
switch err {
Copy link
Collaborator

Choose a reason for hiding this comment

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

are the errors from the watcher and aggWatcher.GetErrors() match ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a revert back to previous implementation. Watch returns watch result and error if any. When I changed the return type of Watch interface, I have to aggregate the watch results and errors. Now the implementation of Watch function will decide how to return error. The auto generated client code calls GetErrors to return error.

@Sindica Sindica force-pushed the aggWatcher-reduce-usage-PR branch from ae690d2 to e523338 Compare October 23, 2020 16:46
Copy link
Collaborator

@yb01 yb01 left a comment

Choose a reason for hiding this comment

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

/lgtm

@zmn223
Copy link
Collaborator

zmn223 commented Oct 23, 2020

/lgtm

@zmn223
Copy link
Collaborator

zmn223 commented Oct 23, 2020

/approve

@centaurus-cloud-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Sindica, yb01, zmn223

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@centaurus-cloud-bot centaurus-cloud-bot merged commit 61b2221 into CentaurusInfra:master Oct 23, 2020
@Sindica Sindica deleted the aggWatcher-reduce-usage-PR branch March 16, 2021 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants