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

Add a new RequeueError type #80

Merged
merged 12 commits into from
Sep 12, 2018
Merged

Add a new RequeueError type #80

merged 12 commits into from
Sep 12, 2018

Conversation

weekface
Copy link
Contributor

@weekface weekface commented Sep 5, 2018

This PR resolves #50 .

@tennix @onlymellb @xiaojingchen @gregwebs PTAL

@weekface
Copy link
Contributor Author

weekface commented Sep 5, 2018

/run-e2e-tests

@gregwebs
Copy link
Contributor

gregwebs commented Sep 5, 2018

It looks like this removes aggregate error creation because they will not pass the test IsRequeueError. However, an error will also not pass that test if it is wrapped in any way. I think we should traverse errors as per https://gist.github.com/gregwebs/5a14a83970d607411310a3e0281d55be.

@gregwebs
Copy link
Contributor

gregwebs commented Sep 5, 2018

I have the function Find now which will work with the k8s aggregate error: https://github.com/lysu/errors/pull/2

@weekface
Copy link
Contributor Author

weekface commented Sep 6, 2018

This sounds good. We should be waiting for that feature.

@weekface weekface changed the title Add a new RequeueError type [WIP] Add a new RequeueError type Sep 7, 2018
@weekface weekface changed the title [WIP] Add a new RequeueError type Add a new RequeueError type Sep 7, 2018
@weekface
Copy link
Contributor Author

weekface commented Sep 7, 2018

Switched to pingcap/errors, @gregwebs PTAL

@@ -216,7 +217,13 @@ func (tcc *Controller) processNextWorkItem() bool {
}
defer tcc.queue.Done(key)
if err := tcc.sync(key.(string)); err != nil {
utilruntime.HandleError(fmt.Errorf("Error syncing TidbCluster %v, requeuing: %v", key.(string), err))
if reqErr := pingcaperrors.Find(err, func(e error) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just do pingcaperrors.Find(err, controller.IsRequeueError)

@gregwebs
Copy link
Contributor

gregwebs commented Sep 7, 2018

I am asking over on pingcap/tidb#7151 about importing pingcap/errors as pkg/errors since we are maintaining backwards compatibility.

@gregwebs
Copy link
Contributor

We are maintaining backwards compatibility for pingcap/errors with pkg/errors, so it is better to import pingcap/errors as pkg/errors as done in TiDB

@tennix
Copy link
Member

tennix commented Sep 11, 2018

/run-e2e-tests

@gregwebs
Copy link
Contributor

/run-e2e-tests

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@tennix tennix merged commit 0537821 into pingcap:master Sep 12, 2018
@weekface weekface deleted the requeue-error branch September 13, 2018 02:10
queenliuxx pushed a commit to queenliuxx/tidb-operator that referenced this pull request Dec 19, 2018
* add RequeueError

* make lint happy

* use pingcap/errors.Find
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.

Need a new error type to requeue the items
3 participants