[HWKMETRICS-800] Validate when scheduled jobs has null values #1006
Conversation
rubenvp8510
force-pushed
the
NPEScheduler
branch
3 times, most recently
from
234f098
to
db6a366
Compare
| return session.execute(insertScheduled.bind(timeSlice, job.getJobId(), job.getJobType(), job.getJobName(), | ||
| job.getParameters().getMap(), getTriggerValue(session, job.getTrigger()))); | ||
| } | ||
|
|
||
| public Observable<ResultSet> updateStatusToFinished(Date timeSlice, UUID jobId) { | ||
| return session.execute(updateStatus.bind((byte) 1, timeSlice, jobId)) | ||
| // First check if the job exists | ||
| return session.executeAndFetch(findByIdAndSlice.bind(timeSlice, jobId)).flatMap( |
There was a problem hiding this comment.
I would consider adding some logging in here. The expectation is that there will be a row for timeSlice and jobId. At a minimum, I would log a warning that the row does not exist. Then the question is, what if anything else should be done?
There was a problem hiding this comment.
I think for now just logging and then ignore it and of course, don't try to do the upsert operation.
rubenvp8510
force-pushed
the
NPEScheduler
branch
from
f997992
to
5469633
Compare
| "slice [%s]", jobId, timeSlice.getTime())); | ||
| // First check if the job exists | ||
| return session.executeAndFetch(findByIdAndSlice.bind(timeSlice, jobId)) | ||
| .isEmpty() |
There was a problem hiding this comment.
I don't have too much experience on javarx, not sure if this is a good way to do the things.
|
@jsanda I've already updated the PR with all comments we talked about. Thanks for taking the time to review this! |
| @@ -195,14 +213,28 @@ public Completable deleteJob(UUID jobId, rx.Scheduler scheduler) { | |||
| } | |||
|
|
|||
| public Observable<ResultSet> insert(Date timeSlice, JobDetails job) { | |||
| if (job.getJobId() == null || job.getJobName() == null || job.getJobType() == null || job.getTrigger() == null) { | |||
| logger.warn("Tried to insert job on scheduled jobs with invalid values"); | |||
There was a problem hiding this comment.
I would include timeSlice and job in the log statement to provide as much info as possible.
| // Fields that shouldn't be null: time_slice, job_id, job_type, job_name, trigger | ||
| private Func1<Row, Boolean> filterAllScheduledQuery = row -> !(row.isNull(0) || row.isNull(1) | ||
| || row.isNull(2) || row.isNull(3) || row.isNull(5)); | ||
|
|
There was a problem hiding this comment.
The functions look good. I'll mention one thing to consider. The filters should help avoid the NPEs but will do so silently. You could add some additional logging at the expense of making the filter functions a bit more verbose, e.g.,
private Func1<Row, Boolean> filterAllScheduledQuery = row -> {
boolean isNull = !(row.isNull(0) || row.isNull(1) || row.isNull(2) || row.isNull(3) || row.isNull(5));
if (isNull) {
// log column names/values...
}
return isNull;
}
rubenvp8510
force-pushed
the
NPEScheduler
branch
2 times, most recently
from
d7dd726
to
4da57f1
Compare
rubenvp8510
force-pushed
the
NPEScheduler
branch
from
4da57f1
to
4dd4142
Compare
rubenvp8510
changed the title
|
I've already addressed all comments, is ready for other review and merge. |
rubenvp8510
changed the title
Fixes https://issues.jboss.org/projects/HWKMETRICS/issues/HWKMETRICS-800