Sync batch operations api #889
Conversation
rodrigozhou
requested review from
mfateev,
Sushisource,
alexshtin,
Spikhalskiy,
bergundy,
cretz and
mmcshane
as code owners
| @@ -76,7 +76,7 @@ import ( | |||
|
|
|||
| "google.golang.org/grpc" | |||
|
|
|||
| workflowservicepb "go.temporal.io/api/workflowservice/v1" | |||
| batchpb "go.temporal.io/api/batch/v1" | |||
There was a problem hiding this comment.
I added the new API module here for the generator.
| @@ -15,10 +15,13 @@ require ( | |||
| github.com/pborman/uuid v1.2.1 | |||
| github.com/robfig/cron v1.2.0 | |||
| github.com/stretchr/testify v1.8.0 | |||
| go.temporal.io/api v1.11.0 | |||
| go.temporal.io/api v1.11.1-0.20220804232755-77e12eef4f2d | |||
There was a problem hiding this comment.
Is this implemented in the server? If not, I think we should wait until then to merge this. Also, I wonder if once implemented in the server we should tag the API since this is such a big change. (but don't tag until you've implemented it as implementation can make things appear that are not otherwise known)
There was a problem hiding this comment.
The server has dummy implementations returning Unimplemented error.
There was a problem hiding this comment.
Why not wait until implemented there to add here? Can reference a local SDK perfect path during development.
This works fine too and how we've done things in the past, I'm just concerned you may have to change the API as you develop.
There was a problem hiding this comment.
I added those to unblock myself since I made changes to the API (same with this PR). Cc: @yux0
There was a problem hiding this comment.
The server implementation PR (for batch API) will be land soon, but we should not block this PR just because server currently return unimplemented as this API is not used anywhere anyway. Server also want to use UpsertMemo feature which is currently blocked on this PR.
There was a problem hiding this comment.
but we should not block this PR just because server currently return unimplemented
Why not? This is a user-facing API update to a non-tagged API version to support unimplemented features.
Server also want to use UpsertMemo feature which is currently blocked on this PR.
This should not block development. I can show how to use go.mod replace clauses to reference a local form of this while you develop. If server wants to use UpsertMemo, that's blocked by #883 where I've also requested we have real tests to even confirm it works. All of this work can be done locally.
There was a problem hiding this comment.
I meant that this PR blocks merging the UpsertMemo PR. I've done all the local tests, and also almost done with writing the integration tests there.
There was a problem hiding this comment.
👍 Yeah, integration tests means things are working which works for me. I'd happily approve/merge. Curious, why does this PR block that one? They seem completely unrelated. (feel free to upgrade API in that PR so it is a standalone unit of work)
There was a problem hiding this comment.
The changes to API related to batch operations affect the generated files grpc_interceptor.go and service_proxy.go. This PR just re-generates those files. It has nothing to do with UpsertMemo, but since my changes also affect those files, I thought it would better to have a PR doing an update for batch related stuff first instead of altogether.
There was a problem hiding this comment.
Sure, you're call, updating API is updating API, could do in same PR or other.
So I can merge this. My concern is that we are giving new public API to end users that is not tagged and is not implemented. But as we've done with other API changes, we can just hope they don't try to use it yet.
Really though, you could do all of this locally in your sdk-go repo and just alter go.mod in server to point to the sdk-go repo locally and do your development there. That is much safer than forcing users to get updates to help our internal dev cycle.
rodrigozhou
force-pushed
the
sync-batch-api-changes
branch
from
01c3ba6 to
b21030d
Compare
What was changed
Batch operations were added in API in temporalio/api#197.
This PR syncs generated files affected by API changes.
Why?
Unblock other changes that depends on recent API changes.
Checklist
make