Implement offset/limit for workflow instance executions #18
Conversation
rouzwawi
force-pushed
the
paginate
branch
4 times, most recently
from
f68f103 to
1bd090f
Compare
|
|
||
| return workflowInstanceDataList; | ||
| return workflowInstancesSet.parallelStream() |
There was a problem hiding this comment.
did the parallelStream() make any discernible difference?
There was a problem hiding this comment.
Yes, on a local run on request that took 9 seconds it went down to about 3.
| final WorkflowId workflowId = WorkflowId.create(componentId, endpointId); | ||
| final List<WorkflowInstanceExecutionData> data; | ||
| final String offset = request.parameter("offset").orElse(""); |
There was a problem hiding this comment.
This is just semantics and I can't really find anything useful on google right now, but isn't this more of a cursor than an offset? At least in the SQL sense, an offset to me is the number of items to skip while this is the element to start from.
| @@ -48,8 +56,12 @@ | |||
| import com.spotify.styx.model.Workflow; | |||
| import com.spotify.styx.model.WorkflowInstance; | |||
| import com.spotify.styx.model.WorkflowState; | |||
| import com.spotify.styx.storage.InMemStorage; | |||
| @@ -382,6 +426,26 @@ public void shouldReturnWorkflowInstanceData() throws Exception { | |||
| assertJson(response, "triggers.[0].executions.[0].statuses.[1].timestamp", is("2016-08-10T07:00:02Z")); | |||
| } | |||
|
|
|||
| @Test | |||
| public void shouldPaginateWorkflowInstancesData() throws Exception { | |||
There was a problem hiding this comment.
Could you add a test where the offset parameter is set as well? I kind of get the feeling that there could be an off-by-one error in there.
scan.setStartRow(Bytes.toBytes(offsetInstance.toKey() + '#'));| workflowInstancesSet.add(wfi); | ||
| if (!Strings.isNullOrEmpty(offset)) { | ||
| final WorkflowInstance offsetInstance = WorkflowInstance.create(workflowId, offset); | ||
| scan.setStartRow(Bytes.toBytes(offsetInstance.toKey() + '#')); |
There was a problem hiding this comment.
Will this include the item named in offset or start from the next one? It's not immediately obvious to me.
There was a problem hiding this comment.
The startRow is inclusive
| return workflowInstancesSet.parallelStream() | ||
| .map(workflowInstance -> { | ||
| try { | ||
| return executionData(workflowInstance); |
There was a problem hiding this comment.
This is only partly related since it wasn't changed in this commit, but this function will call readEvents which in turns reads from the same table we just read from to get this list. Is there a point to that? Why not just use that data in the first scan?
| @@ -73,7 +73,7 @@ | |||
| /** | |||
| * Removes a workflow definition. | |||
| * | |||
| * @param workflowId The workflowid to remove. | |||
| * @param workflowId The workflow id to remove. | |||
There was a problem hiding this comment.
Let's use the type instead?
| * @param workflowId The workflowId to get execution information for | ||
| * @param offset The offset parameter |
There was a problem hiding this comment.
The id of the last item from previous fetch. Or something to that effect.
| storage.writeEvent(SequenceEvent.create(Event.triggerExecution(WFI3, "triggerId3"), 0L, 3L)); | ||
|
|
||
| List<WorkflowInstanceExecutionData> workflowInstanceExecutionData = | ||
| storage.executionData(WORKFLOW_ID1, WFI2.parameter(), 100); |
There was a problem hiding this comment.
Ah! So the offset parameter is inclusive. This is unexpected to me and not how pagination works to me. You would have to guess the next ID to not get duplicates between pages.
rouzwawi
force-pushed
the
paginate
branch
3 times, most recently
from
4660521 to
09ec0a8
Compare
|
Discussion offline: we'll merge this for now and iterate on the details. |
No description provided.