Scala Client: add new jobWatcher() method for watcher API#4606
Scala Client: add new jobWatcher() method for watcher API#4606richscott merged 9 commits intoarmadaproject:masterfrom
Conversation
Add new jobWatcher(queue, jobset, lastMessage) method to ArmadaClient class,
based on original logic in armada-spark repository.
Fill in implementation of watch() method in ArmadaClientSuite (test)
EventMockServer. Clear out job, queue, and status maps (the backing store for
the ArmadaMockServer) before running each test, to avoid test pollution.
Change from using .onComplete{..} idiom to Await.result(response, timeout) for
more robust async testing behavior. Add test for jobWatcher(), verify that
expected events do arrive and in expected order.
Fixes G-Research/spark#160
Signed-off-by: Rich Scott <richscott@sent.com>
| matchingJobs.foreach { case (jobId, job) => | ||
| // Check if job exists in statusMap | ||
| statusMap.get(jobId) match { | ||
| case Some(currentStatus) => |
There was a problem hiding this comment.
it doesn't appear as currentStatus is used anywhere. Shouldn't it be?
Also, it looks like you are emiting all the "usual" states simultaneously below. That seems odd. Don't we want the state emitted to depend on the current state?
There was a problem hiding this comment.
The currentStatus was just a placeholder for whatever get(jobId) returned, but it was ultimately just for determining if the given job ID really existed in the mock Armada. I have changed this to just use Map.contains() instead, which makes the code simpler, in commit 38306bb.
The state events are emitted sequentially (but not simultaneously, strictly-speaking), mostly just to verify that they are received by the iterator that jobWatcher(..) returns (and is used in armada-spark). The code just checks that all the events are received and in the order they were emitted. There's no higher-level state-transition logic in here for ordering, valid transition states, etc - that's elsewhere in Armada (outside the client), so it didn't seem necessary to try to reproduce and test that here.
Also, the e2e test in armada-spark appears to run successfully with a change of using the new jobWatcher() API method, but I will wait until this is merged and published out to the world.
There was a problem hiding this comment.
I'm ok with it, but it is puzzling to see all events being emitted sequentially. I would add some of the comments from the explanation above explaining why it is the way it is.
There was a problem hiding this comment.
👍🏼 I've added an explanatory note in commit 9b0e647 - let me know if seems incomplete or confusing.
Replace Map.get(jobId) and check for optional return with just a Map.contains(jobId) - we don't really care about the value (status), just that it's a valid job ID. Signed-off-by: Rich Scott <richscott@sent.com>
Signed-off-by: Rich Scott <richscott@sent.com>
GeorgeJahad
left a comment
There was a problem hiding this comment.
lgtm, thanks @richscott !
…ect#4606) Add new jobWatcher(queue, jobset, lastMessage) method to ArmadaClient class, based on original logic in armada-spark repository. Fill in implementation of watch() method in ArmadaClientSuite (test) EventMockServer. Clear out job, queue, and status maps (the backing store for the ArmadaMockServer) before running each test, to avoid test pollution. Change from using .onComplete{..} idiom to Await.result(response, timeout) for more robust async testing behavior. Add `watch` support to testing mock server, add unit test for jobWatcher(). Fixes G-Research/spark#160 --------- Signed-off-by: Rich Scott <richscott@sent.com> Signed-off-by: Sigele Nickerson-Adams <sigele.nickerson-adams@nmc2.ai>
Add new jobWatcher(queue, jobset, lastMessage) method to ArmadaClient class, based on original logic in armada-spark repository.
Fill in implementation of watch() method in ArmadaClientSuite (test) EventMockServer. Clear out job, queue, and status maps (the backing store for the ArmadaMockServer) before running each test, to avoid test pollution.
Change from using .onComplete{..} idiom to Await.result(response, timeout) for more robust async testing behavior. Add
watchsupport to testing mock server, add unit test for jobWatcher().Fixes G-Research/spark#160