-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Code] fixed the issue that the repository can not be deleted in some cases. #42841
Conversation
Pinging @elastic/code |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fixes all make sense. Thanks Yang. see my comments inline.
x-pack/legacy/plugins/code/server/queue/cancellation_service.ts
Outdated
Show resolved
Hide resolved
@@ -68,6 +68,10 @@ export class CancellationSerivce { | |||
// Try to cancel the job first. | |||
await this.cancelJob(jobMap, repoUri); | |||
jobMap.set(repoUri, { token, jobPromise }); | |||
// remove the record from the cancellation service when the promise is fulfilled or rejected. | |||
jobPromise.finally(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is indeed better to remove the job from the service map. thanks.
a unit test for CancellationService when the promise of the job get rejected.
💔 Build Failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
); | ||
// make sure the promise won't be fulfilled immediately | ||
const promise = new Promise(resolve => { | ||
setTimeout(resolve, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the best way to manipulate the timer in unit test is using sinon's fake timer (example here). Otherwise, the test will have some risks of flaky. It's up to you if you want to update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention here is to make sure the promise is completed after service.cancelIndexJob(repoUri)
is called, otherwise the promise will be removed from the map. I tried another way to defer its resolution and rejection, as introduced in this question. It seems a bit not clean here, any better ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some other options
- use
Promise.deferred
, basically it's same as your current approach but the API is obsolete - use third party library like https://www.npmjs.com/package/delay, which is basically your previous approach
so overall, I think your current solution is good enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fantapsody My point here is not the way you defer the promise wrong. I should be clear here. Even if you defer 100ms here, there are still some chances that the promise resolves before the cancelIndexJob
. A better way is to create a faketimer and then tick only 50ms to guarantee this won't happen.
await service.registerCancelableIndexJob(repoUri, token as CancellationToken, promise);
// Tick 50ms so it's 100% sure that the promise won't complete before the next statement
faketimer.tick(50)
await service.cancelIndexJob(repoUri);
Again, it's up to you if you want to make the change or not. no strong opinion here. Just introduce you this powerful faketimer tool since you will probably need this sooner or later.
💚 Build Succeeded |
); | ||
// make sure the promise won't be fulfilled immediately | ||
const promise = new Promise(resolve => { | ||
setTimeout(resolve, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fantapsody My point here is not the way you defer the promise wrong. I should be clear here. Even if you defer 100ms here, there are still some chances that the promise resolves before the cancelIndexJob
. A better way is to create a faketimer and then tick only 50ms to guarantee this won't happen.
await service.registerCancelableIndexJob(repoUri, token as CancellationToken, promise);
// Tick 50ms so it's 100% sure that the promise won't complete before the next statement
faketimer.tick(50)
await service.cancelIndexJob(repoUri);
Again, it's up to you if you want to make the change or not. no strong opinion here. Just introduce you this powerful faketimer tool since you will probably need this sooner or later.
… cases. (elastic#42841) * [Code] fixed the issue that the repository can not be deleted in some cases. * [Code] catch the rejected exception while cancelling the job, and added a unit test for CancellationService when the promise of the job get rejected. * [Code] refined tests for CancellationService by deferred completion of the promise.
… cases. (#42841) (#43001) * [Code] fixed the issue that the repository can not be deleted in some cases. * [Code] catch the rejected exception while cancelling the job, and added a unit test for CancellationService when the promise of the job get rejected. * [Code] refined tests for CancellationService by deferred completion of the promise.
…p-metrics-selectall * 'master' of github.com:elastic/kibana: (306 commits) [ML] Adding job overrides to the module setup endpoint (elastic#42946) [APM] Fix missing RUM url (elastic#42940) close socket timeouts without message (elastic#42456) Upgrade elastic/charts to 8.1.6 (elastic#42518) [ML] Delete old AngularJS data visualizer and refactor folders (elastic#42962) Add custom formatting for Date Nanos Format (elastic#42445) [Vega] Shim new platform - vega_fn.js -> vega_fn.js , use ExpressionFunction (elastic#42582) add socket.getPeerCertificate to KibanaRequest (elastic#42929) [Automation] ISTANBUL PRESET PATH is not working fine with constructor(private foo) (elastic#42683) [ML] Data frames: Updated stats structure. (elastic#42923) [Code] fixed the issue that the repository can not be deleted in some cases. (elastic#42841) [kbn-es] Support for passing regex value to ES (elastic#42651) Connect to Elasticsearch via SSL when starting kibana with `--ssl` (elastic#42840) Add Elasticsearch SSL support for integration tests (elastic#41765) Fix duplicate fetch in Visualize (elastic#41204) [DOCS] TSVB and Timelion clean up (elastic#42953) [Maps] [File upload] Fix maps geojson upload hanging on index step (elastic#42623) [APM] Use rounded bucket sizes for transaction distribution (elastic#42830) [yarn.lock] consistent resolve domain (elastic#42969) [Uptime] [Test] Repurpose unit test assertions to avoid flakiness (elastic#40650) ...
Addresses the issue https://github.com/elastic/code/issues/1500.
Summary
Summarize your PR. If it involves visual changes include a screenshot or gif.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] Unit or functional tests were updated or added to match the most common scenarios[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately