SERV-505 #26
SERV-505 #26
Conversation
# This is the 1st commit message: SERV-505 Added jobsQueue to watcherVerticle # This is the commit message #2: [SERV-505] Addition of JobsQueue Class and Test # This is the commit message #3: [SERV-505] Consistency update to CsvItemTest.java # This is the commit message #4: [SERV-505] Cleaned up JobsQueue code # This is the commit message #5: [SERV-505] Completion of JobsQueue functionality and added to WatcherVerticle # This is the commit message #6: [SERV-505] Added in messages # This is the commit message #7: [SERV-505] Fix checkstyle and delete unnecessary files # This is the commit message #8: [SERV-505] Fixed checkstyle violations
| /** | ||
| * Name of shared map | ||
| */ | ||
| private static final String MY_JOBS_QUEUE = "jobs-queue"; |
There was a problem hiding this comment.
It's fine if you like, but static field names don't need to start with "my" (only instance fields). Just FYI.
You could also change the visibility to public so that the test could directly reference it rather than redefine it 🤷🏻
|
|
||
| // Associate handlers with operation IDs from the application's OpenAPI specification | ||
| routerBuilder.operation(Op.GET_STATUS).handler(new StatusHandler(getVertx())); | ||
| //add for JobsStatusHandler |
There was a problem hiding this comment.
Assuming this is a "to-do", it would be good to explicitly mark it as such by prefixing with TODO (you may have noticed that many IDEs automatically aggregate such comments in a special "problems" area of their UI).
| assertEquals(PATH_ROOT, new CsvItem().setPathRoot(SOUL_WAV).getPathRoot()); | ||
| final CsvItem csvItem = new CsvItem(); | ||
|
|
||
| csvItem.setPathRoot(PATH_ROOT); | ||
| assertEquals(PATH_ROOT, csvItem.getPathRoot()); |
There was a problem hiding this comment.
Why the change in argument passed to setPathRoot?
| /* | ||
| * A smaller sample audio CSV | ||
| */ | ||
| public static final String SOUL_SAMPLE = "job_map_test.csv"; | ||
|
|
There was a problem hiding this comment.
Did you mean to check in a new CSV?
|
|
||
|
|
||
| /** | ||
| * An abstract test that other tests can extend. |
| myJobsQueue.addJobToQueue(item); | ||
|
|
There was a problem hiding this comment.
I think this statement is misplaced. Here, it will be executed when all of the tasks in the futures List succeed (i.e., after all the jobs have finished). IMO, it should be at the end of the lambda passed to reader.readBeans().forEach(...) above.
|
|
||
| myJobsQueue.removeJobInQueue(ark); | ||
|
|
There was a problem hiding this comment.
Wondering if this one is also misplaced. Is there a reason not to run this for each record (i.e., unconditionally in each loop iteration)?
| jobsMap.put(ARK_REM, aItem, result -> { | ||
| if (result.failed()) { | ||
| aContext.fail(result.cause()); | ||
| } | ||
| }); | ||
|
|
||
| final Future<Void> future = JobsQueue.removeJobInQueue(ARK_REM); | ||
| jobsMap.get(ARK_REM).onFailure(failure -> { | ||
| aContext.fail(); | ||
| }).onSuccess(res -> { | ||
| assertEquals(res, null); | ||
| asyncTask.complete(); | ||
| }); |
There was a problem hiding this comment.
In this special case, these three async tasks (jobsMap.put, JobsQueue.removeJobInQueue, jobsMap.get) get coordinated properly, but that's just because they happen to use the same nice and lawful Vert.x core asynchronous API all the way down: the operations are very fast, so the order in which they start happens to be the same as the order in which they finish. This will not be the case in general though (for example, if you have an async method that runs some long blocking I/O on a worker thread). So I'd argue it's always preferable to explicitly coordinate async stuff:
| jobsMap.put(ARK_REM, aItem, result -> { | |
| if (result.failed()) { | |
| aContext.fail(result.cause()); | |
| } | |
| }); | |
| final Future<Void> future = JobsQueue.removeJobInQueue(ARK_REM); | |
| jobsMap.get(ARK_REM).onFailure(failure -> { | |
| aContext.fail(); | |
| }).onSuccess(res -> { | |
| assertEquals(res, null); | |
| asyncTask.complete(); | |
| }); | |
| jobsMap.put(ARK_REM, aItem, result -> { | |
| if (result.failed()) { | |
| aContext.fail(result.cause()); | |
| } else { | |
| JobsQueue.removeJobInQueue(ARK_REM).compose(nil -> { | |
| return jobsMap.get(ARK_REM); | |
| }).onFailure(details -> { | |
| aContext.fail(details); // Also, passing in the failure cause is a good idea | |
| }).onSuccess(res -> { | |
| assertEquals(null, res); // Also also, the expected value should always go first | |
| asyncTask.complete(); | |
| }); | |
| }); | |
| }); |
To be fair, this is really cumbersome since the void-returning async methods are all that's available with these older versions of Vert.x.
There was a problem hiding this comment.
Oh, I just realized this app uses Vert.x 4, in which case you could use the Future-returning versions if you wanted Javadocs in case you're interested:
Co-authored-by: Mark A. Matney, Jr. <[email protected]>
3 participants
Addition of a jobs queue class and testing implemented through the shared async map