Time Skipping Service #1076
Time Skipping Service #1076
Conversation
Spikhalskiy
force-pushed
the
time-skipping-api
branch
9 times, most recently
from
7968e9f to
8a109bc
Compare
Spikhalskiy
requested review from
Sushisource,
bergundy,
cretz,
mfateev and
mmcshane
as code owners
|
@bergundy This is the most important for you for time-skipping in Typescript SDK: https://github.com/temporalio/sdk-java/pull/1076/files#diff-d7a107df90fe446efc696d828e3df42e829262e802060ade229d4cdddbb6fb26R41 |
| result.complete(null); | ||
| }, | ||
| "workflow sleep"); | ||
| workflowStore.getTimer().unlockTimeSkipping("TestWorkflowService sleep"); |
There was a problem hiding this comment.
Does this mean that even if a client has requested that time skipping be locked, it will implicitly be unlocked by any sleep call? Why wouldn't we push the need for this to the client?
There was a problem hiding this comment.
The only situation when we need to use sleep is when the time skipping is locked. Otherwise time fast forwards without limits, no sleeps is needed.
What happens here, the timer gets scheduled. After that, the time gets unlocked. After that time gets locked by the fired timer that we just scheduled.
| public static Builder newBuilder(ServiceStubsOptions options) { | ||
| return options instanceof TestServiceStubsOptions | ||
| ? newBuilder((TestServiceStubsOptions) options) | ||
| : new Builder(options); |
There was a problem hiding this comment.
Am I misreading or do both branches here do the same thing?
There was a problem hiding this comment.
You are technically correct.
This structure was copied from WorkflowServiceStubsOptions and it makes sense there (you can create a builder from the WorkflowServiceStubsOptions or from ServiceStubsOptions which can be from some other type of stubs). Here it doesn't do much, but I don't want to remove it. The only reason it doesn't make sense right now is that the TestServiceStubsOptions doesn't add any fields to ServiceStubsOptions.
There was a problem hiding this comment.
On the second thought... let me restructure to both leave the framework in place and don't have such a strange-looking code.
| // we can't continue doing that. If there is at least one more flag or parameter we should | ||
| // incorporate a framework like picocli | ||
| if (args.length > 1) { | ||
| if ("--enable-time-skipping".equalsIgnoreCase(args[1])) { |
There was a problem hiding this comment.
I'm tickled that I can do --EnAbLe-TiMe-SkIpPiNg
| this.testServiceStubs = testServiceStubs; | ||
| } | ||
|
|
||
| public void lockTimeSkipping(String caller) { |
There was a problem hiding this comment.
There may be a problem with this approach - when a given call to lockTimeSkipping returns, it is not guaranteed that time skipping has been locked. Any call subsequent to the first call might return early while that first call is still executing the RPC call. This leaves a window where this method has returned but time skipping is not (yet) locked.
There was a problem hiding this comment.
This (return without lock/unlock actually happening) actually shouldn't be an issue for the caller code. The only thing this wrapper is ensuring is that one WorkflowStub doesn't cause several locks/unlocks. Time can be double/triple/etc locked, so even the return of the RPC doesn't mean that the time was actually locked or unlocked after it.
So, while this comment is easy to address (by adding a lock), I don't think it needs to be.
There is another problem with this code though: #392
BTW, this piece is a carry-over and refactoring of an existing code (https://github.com/temporalio/sdk-java/blob/4678ae47ea59d873e0c24c460ec8f21a8965e8f3/temporal-testing/src/main/java/io/temporal/testing/TestWorkflowEnvironmentInternal.java#L266-L265)
temporal-test-server/src/main/java/io/temporal/internal/testservice/TestService.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * @deprecated use {@link io.temporal.serviceclient.TestServiceStubs} and {@link | ||
| * io.temporal.api.testservice.v1.TestServiceGrpc.TestServiceBlockingStub#getCurrentTime(Empty)} | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Are users using these directly and that's why we need to deprecate them?
There was a problem hiding this comment.
They shouldn't, because it's in internal package and has comments to don't use it directly. But I know some users who were playing with the test server quite a bit maybe using and exposing them in their own wrappers, yes.
I'm going to clean this up next time I touch code around it (even if it's the next release)
temporal-test-server/src/main/java/io/temporal/serviceclient/TestServiceStubsImpl.java
Outdated
Show resolved
Hide resolved
| int port = Integer.parseInt(args[0]); | ||
| boolean enableTimeSkipping = false; | ||
|
|
||
| // we can't continue doing that. If there is at least one more flag or parameter we should |
There was a problem hiding this comment.
Lol, I love the self-awareness of this comment.
temporal-test-server/src/main/proto/temporal/api/testservice/v1/service.proto
Outdated
Show resolved
Hide resolved
temporal-test-server/src/main/proto/temporal/api/testservice/v1/service.proto
Outdated
Show resolved
Hide resolved
temporal-test-server/src/main/proto/temporal/api/testservice/v1/service.proto
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| public void unlockTimeSkipping(String caller) { |
There was a problem hiding this comment.
If there are other clients not using this idempotent locker, when this returns time also might not be un-locked. Is that a potential issue? Is the expectation that, if you're using this class, every lock/unlock goes through this class?
There was a problem hiding this comment.
No, it's not a potential issue. Time is not guaranteed to be unlocked when this method returns.
This IdempotentLocker only guarantees that time will be locked/unlocked once for a WorkflowStub (see TimeLockingInterceptor where it's used), it doesn't guarantee that the whole test process locks/unlocks only once. Quite opposite, time locking feature is counting, so it's explicitly supported that it can be locked several times. And we actually use it. For example, to disable time locking by default, we just call lockTimeSkipping WITHOUT a correspondent unlock (see https://github.com/temporalio/sdk-java/pull/1076/files#diff-ce2ffc1d1e2ae7b11577efed9e362e7148f6304a85da5e78ba6b3b45a896f15eR101)
Spikhalskiy
force-pushed
the
time-skipping-api
branch
4 times, most recently
from
b84f950 to
aed4684
Compare
|
@Sushisource @mmcshane completely reformulated docs on TestService (https://github.com/temporalio/sdk-java/pull/1076/files#diff-d7a107df90fe446efc696d828e3df42e829262e802060ade229d4cdddbb6fb26R39) |
Spikhalskiy
force-pushed
the
time-skipping-api
branch
from
aed4684 to
98ef570
Compare
Spikhalskiy
force-pushed
the
time-skipping-api
branch
from
98ef570 to
f8e473f
Compare
3 participants
What was changed
Test Service gRPC service is added with time-skipping methods.
JavaSDK is fully reworked to use Time Skipping gRPC Stubs instead of accessing the in-memory TestService internal instances and classes directly when time skipping functionality is needed, reducing coupling between SDK test classes and test-server implementation.
Native Test Service default starting behavior changed to start with locked (disabled) time-skipping to behave closer to a real Temporal Server by default.