Conversation
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
c310dc2 to
8d35170
Compare
snazy
changed the title
pingtimeout
left a comment
pingtimeout
left a comment
There was a problem hiding this comment.
Thanks @snazy for the thorough work ! The PR looks good to me. I have made some suggestions for Javadoc clarity. I consider them all as nit. Feel free to disregard any that is not valuable enough.
| @PolarisImmutable | ||
| @JsonSerialize(as = ImmutableTaskBehaviorId.class) | ||
| @JsonDeserialize(as = ImmutableTaskBehaviorId.class) | ||
| public interface TaskBehaviorId { |
There was a problem hiding this comment.
I suspect that the use of an interface here instead of a record is to allow extensibility, as records cannot be extended from?
There was a problem hiding this comment.
No specific reason, but all other types are interfaces.
| import java.lang.annotation.Target; | ||
|
|
||
| /** | ||
| * Provide the {@linkplain TaskBehaviorId task behavior ID} using the |
There was a problem hiding this comment.
| * Provide the {@linkplain TaskBehaviorId task behavior ID} using the | |
| * Provides the {@linkplain TaskBehaviorId task behavior ID} using the |
Is this sentence correct? This javadoc is identical to that of TaskResultTypeId, but neither class uses nor are used by the TaskBehaviorId class at all.
There was a problem hiding this comment.
True, this part is currently not wired anywhere.
It will be used to dynamically find the right implementations as implementations are pluggable. It is later used in the async & reliable tasks implementation to find the right Jackson serializer/deserializer (Jackson polymorphism).
There was a problem hiding this comment.
Ok so then would it make sense for the Javadoc to be "Provides the Task Parameter ID using the Jackson-polymorphism-mechanism" instead of "Provides the task behavior ID using the Jackson-polymorphism-mechanism"?
I am not sure how it will be used. I suppose this and the TaskResultTypeId annotation could be used to find a task behaviour which uses matching parameter id and result type id. In which case the Javadoc should still be updated as the annotation does not directly provide the task behaviour id. Am I missing something?
There was a problem hiding this comment.
I think it will become clearer with the implementation. In case of any doubts we can certainly change it later.
|
|
||
| @SuppressWarnings("UnnecessaryDefault") | ||
| @JsonIgnore | ||
| default boolean isScheduled(Instant now) { |
There was a problem hiding this comment.
It seems to me that this method returns true if the current task is scheduled in the future. However it it is SCHEDULED but the current date is after the scheduled date, then this method returns false. Is this intended? Maybe add a Javadoc to explain how this method is expected to be used?
There was a problem hiding this comment.
Technically, this function is used to determine whether a task can be scheduled. Updated this a bit.
There was a problem hiding this comment.
My point is that a task status can be SCHEDULED and the method name isSchedule() suggests that it is a shortcut for status() == SCHEDULED. But the method does something else. Maybe it could be good naming it canBeScheduled() given your answer?
There was a problem hiding this comment.
Seen the commit from Dec 20?
| * commit-retries happen. Therefore {@code updater} function must be free of side effects. | ||
| * | ||
| * @param updater Update function must be idempotent and expect to be invoked multiple times. Gets | ||
| * the current state as its input may throw |
There was a problem hiding this comment.
* @param updater the update function, which must be idempotent and expect to be invoked multiple
* times. It receives the current state as input and returns the desired change. It may
* throw an exception in the event of a failure.
I assume the semantics here are as follows:
- If the updater function returns a
TaskStoreResult, this new state should be persisted to the task store - If it returns an empty optional, then the task should be deleted from the task store
- If it throws, then an error occurred and an updated state should be recomputed
Is this correct? Those semantics should probably be described in the Javadoc.
There was a problem hiding this comment.
Mostly.
A TaskStateUpdater function returns a TaskChange "instruction" (no change, create-or-update or update-and-unschedule).
There is no "delete" passed back.
It's the TaskStore's responsibility to perform the actual change and yield the outcome.
Just the interfaces and value types needed for "Async & reliable tasks", the first three modules as described in `tasks/README.md`. The 3 different modules serve different "audiences": * API is for call sites that submit tasks via the `Tasks` interface. * SPI is for task/behavior implementations * Store is for persistence abstraction, only for tasks implementations The only entry point is the `org.apache.polaris.tasks.api.Tasks` interface with a function defining the behavior and providing a parameter object (if necessary), returning a `TaskSubmission`. Call sites _may_ subscribe to a `CompletionStage`, but the idea is that it's rather "fire and forget" and the task behavior does "everything that's needed". This allows the task to be executed on any node. There's no guarantee in any form that a task will run "locally" or any other specific node. Every Polaris node can handle task execution and perform failure/retry handling. Polaris nodes may use a "server" implementation or a "client" implementation or a "remote" implementation - that's defined upon deployment or by configuration (TBD). Implementations would be abstracted similarly: * Store implementation with NoSQL specifics, especially for the indexed/sorted lookups * Store implementation with JDBC specifics (can leverabe `BasePersistence`) * Tasks implementation for servers, clients and remote
This branch has not been deployed
4 participants
Just the interfaces and value types needed for "Async & reliable tasks", the first three modules as described in
tasks/README.md. The 3 different modules serve different "audiences":Tasksinterface.The only entry point is the
org.apache.polaris.tasks.api.Tasksinterface with a function defining the behavior and providing a parameter object (if necessary), returning aTaskSubmission. Call sites may subscribe to aCompletionStage, but the idea is that it's rather "fire and forget" and the task behavior does "everything that's needed". This allows the task to be executed on any node. There's no guarantee in any form that a task will run "locally" or any other specific node. Every Polaris node can handle task execution and perform failure/retry handling. Polaris nodes may use a "server" implementation or a "client" implementation or a "remote" implementation - that's defined upon deployment or by configuration (TBD).Implementations would be abstracted similarly:
BasePersistence)