-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: support Diregapic long running operation #1119
base: main
Are you sure you want to change the base?
Conversation
src/longRunningCalls/longrunning.ts
Outdated
): void; | ||
} | ||
|
||
// TODO(summerji): check with Vadym the error compose; |
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.
Leaving a comment to make sure this does not get checked in :)
src/longRunningCalls/longrunning.ts
Outdated
} | ||
} | ||
|
||
_isComputeOperationDone(op: ComputeLROOperation): boolean { |
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 method should either be static or be defined outside of the class since it never accesses this
.
@@ -129,13 +160,14 @@ export class Operation extends EventEmitter { | |||
* request. | |||
*/ | |||
cancel() { | |||
if (this.currentCallPromise_) { | |||
this.currentCallPromise_.cancel(); | |||
if (!this.diregapic && this.currentCallPromise_) { |
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.
I believe there is no cancellation for diregapic. Can we just throw?
if (this.diregapic) {
throw new Error("Cancellation is not supported for this method");
}
@vam-google what do you think?
src/longRunningCalls/longrunning.ts
Outdated
// Polling request require fields has been checked in the LRO method before | ||
// operation.promise() has been called. Transcoding will throw error if any required | ||
// fields missing. | ||
_composeComputeRequest(): ComputeOperationRequest { |
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.
Summoning @vam-google to check the logic of this method.
@@ -698,3 +773,512 @@ describe('longrunning', () => { | |||
}); | |||
}); | |||
}); | |||
|
|||
describe('diregapic longrunning', () => { |
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.
May I ask you to move this to a separate file? 1000+ lines is a lot :) This will also simplify all the test initialization code since you won't need to do if (diregapic)
in it.
Thanks @summer-ji-eng, it looks great! Left some comments, mostly style+readability. I'll defer reviewing the DIREGAPIC-specific logic to @vam-google. |
c25e9cf
to
84be951
Compare
Before the implementation, diregapic LRO return unary with
compute.v1.Operation
. It require manual code to poll the operation until it finish.After, the sample code to call insert LRO method
It support Compute 4 operation polling service:
GlobalOperations
RegionOperations
ZonalOperations
GlobalOrganizationOperations
This version is not support dynamic diregapic lro, will modify with gapic-generator-typescript recognize diregapic LRO with annotation.