check connection worker on temporal #2312
Conversation
| if (run.getStatus() == JobStatus.SUCCEEDED) { | ||
| Preconditions.checkState(run.getOutput().isPresent()); | ||
| LOGGER.info("job output {}", run.getOutput().get()); | ||
| return run.getOutput().get(); |
| .withJobId(jobId) | ||
| .withAttemptId((long) attempt) | ||
| .withDockerImage(config.getDockerImage()); | ||
| try { |
| LOGGER.info("job output {}", run.getOutput().get()); | ||
| return run.getOutput().get(); | ||
| } else { | ||
| throw new TemporalJobException(); |
There was a problem hiding this comment.
I don't think this is going to work. You're catching and re-throwing as a runtime
| .resolve(String.valueOf(launcherConfig.getJobId())) | ||
| .resolve(String.valueOf(launcherConfig.getAttemptId().intValue())); | ||
|
|
||
| MDC.put("job_id", String.valueOf(launcherConfig.getJobId())); |
There was a problem hiding this comment.
Are you planning to DRY the MDC config?
There was a problem hiding this comment.
yeah. jared just did it in his PR. i'm hoping we can do a base class of some sort so that we don't need to have to remember to do these over and over again.
| } else { | ||
| throw new RuntimeException(String.format("Spec job subprocess finished with exit code %s", exitCode)); | ||
| throw new TemporalJobException(); |
There was a problem hiding this comment.
Can you add some tests for these classes? That would likely have caught this bug
| try { | ||
| return new OutputAndStatus<>(JobStatus.SUCCEEDED, supplier.get()); | ||
| } catch (TemporalJobException e) { | ||
| return new OutputAndStatus<>(JobStatus.FAILED); |
There was a problem hiding this comment.
This will never happen based on the worker code
| private OutputAndStatus<JobOutput> toOutputAndStatus(CheckedSupplier<JobOutput, TemporalJobException> supplier) { | ||
| try { | ||
| return new OutputAndStatus<>(JobStatus.SUCCEEDED, supplier.get()); | ||
| } catch (TemporalJobException e) { |
There was a problem hiding this comment.
Since you're also throwing runtime, what do you expect the behavior for the output ?
There was a problem hiding this comment.
yup. the exception handling is not good right now. i will be looking at it across the board today and fix these.
What
Pre-merge Checklist
Recommended reading order
test.javacomponent.tsApproach
@jrhizor how do you feel about this approach to migration. it leaves the workers intact, which I think makes refactoring after the fact a little easier. It also gives us more ability to do whatever generic temporal base class we might want to do to abstract away like setting up log paths, MDCs, etc (for now we are just duplicating that logic). It also means we don't need to re do a lot of tests since the Workers are already well tested.
the one annoying downside is that Workers output OutputAndStatus which is not serializable, so we need to unwrap it and then rewrap it (see CheckConnectionWorkflow and TemporalClient). This is annoying, but once all workers are on temporal we can just remove OutputAndStatus from the worker iface. wdyt?
(update, jared and I discussed offline and agreed that this is okay as an intermediate step.)