P/mike/job history upstream #402
Conversation
|
Single successful job run Currently executing job If there is no past data, no UI block is shown for this. This makes this feature backwards compatible. If there is a data outage (cassandra down), no data is also shown. Number of past job runs are limited by a parameter, the default value is 5. If the screen is shrunk, the UI adapts appropriately |
|
The purpose of this feature is to show the past task executions for each job (if data exists). Data is extracted from the existing Cassandra setup. The tasks are reverse sorted by date, with the most recent at the top. There are two commits for this request. The commits are separated to show the initial change to the cassandra table schema by changing the indexing scheme. To adapt to the existing schema that upstream uses, I changed to go from a partition key to a secondary index. After getting feedback, I'll squash the commits and merge the final version. |
|
@brndnmtthws @elingg - since this is the same area as the last pull request, can you please review again? thanks :) |
|
|
||
| json.writeEndObject() | ||
| } | ||
| } |
|
Looks great, minus some minor comments. Can you also squash & rebase into 1 commit, please. Thanks! |
|
Another thing: can you update the README to explain how this works? I'm worried some folks (who don't use the C* stuff) will be confused as to why they don't have this data. |
mngo87
force-pushed
the
p/mike/job_history_upstream
branch
3 times, most recently
from
90e0a1a
to
1e47bc7
Compare
|
@brndnmtthws - comments addressed. can you review again? |
| val query = s"SELECT * FROM ${config.cassandraTable()} WHERE ${JOB_NAME}='${jobName}';" | ||
| val prepared = statements.getOrElseUpdate(query, { | ||
| var level = ConsistencyLevel.ANY | ||
| if (ConsistencyLevel.ANY.name().equalsIgnoreCase(config.cassandraConsistency())) { |
There was a problem hiding this comment.
Maybe pull this check out into a separate method like readConsistencyLevel()? Then have:
session.prepare(
new SimpleStatement(query)
.setConsistencyLevel(readConsistencyLevel())
.asInstanceOf[RegularStatement]
)
|
One minor nit pick. Any comments from @elingg? |
|
Thanks for the contribution @mngo87 and for the review @brndnmtthws, I'll take a final look at this ASAP. |
mngo87
force-pushed
the
p/mike/job_history_upstream
branch
from
1e47bc7
to
166d145
Compare
|
Sure no problem. I updated once more to address @brndnmtthws 's comments |
| @@ -310,4 +447,15 @@ class JobStats @Inject()(clusterBuilder: Option[Cluster.Builder], config: Cassan | |||
| } | |||
|
|
|||
| } | |||
|
|
|||
| private def readConsistencyLevel() : ConsistencyLevel = { | |||
| var level = ConsistencyLevel.ANY | |||
There was a problem hiding this comment.
You can omit this level var altogether. The last statement in the function will be the return value.
There was a problem hiding this comment.
good point. will update
mngo87
force-pushed
the
p/mike/job_history_upstream
branch
from
166d145
to
e6bd023
Compare
|
Looks great now, thanks. |
| var slaveId = row.getString(SLAVE_ID) | ||
| var isFailure = row.getBool(IS_FAILURE) | ||
|
|
||
| var taskStats = taskMap.getOrElseUpdate(taskId, new TaskStat(taskId, jobName, slaveId)) |
There was a problem hiding this comment.
nit: this variable should probably be named taskStat
There was a problem hiding this comment.
hah good catch. will update.
|
This is really great work, @mngo87! My main concern at this point is with clean code, specifically in the method, getParsedJobStatsByName. I want to make sure we have a clean way of updating the appropriate fields in TaskStat and loading and saving the object to DB. Right now we modify different fields at different times, which is not ideal and difficult to read. |
|
Thanks @elingg for the review. Let me know your thoughts on what I said. Just to re-iterate, TaskStat is just merely a object representation of the chronos table. It will pull all the data at once and then parse. It will not get the subsequent updates during parsing. That can be a feature for later. |
|
Thanks for the quick response @mngo87! I left my reply above. That's fine that we don't persist the TaskStat or the Map, but I would like to clean up that method for simplicity and readability. |
mngo87
force-pushed
the
p/mike/job_history_upstream
branch
2 times, most recently
from
694f41c
to
bd6c68c
Compare
|
@elingg - I just updated the request. Instead of dealing with when to clear the taskMap, I decided to go back to the disposable map model and use it for temporary storage. i separated out the code. Let me know what you think |
|
@mngo87, with the cleaner logic, the disposable map makes more sense. I think it is a reasonable design. Nice work! I just made one comment to address, but it LGTM |
|
@elingg - let me know your thoughts on my comments and I'll address if needed. Thanks for the review. |
|
Responded, thanks! |
…ent and past runs
mngo87
force-pushed
the
p/mike/job_history_upstream
branch
from
bd6c68c
to
1f72b4d
Compare
|
@elingg - updated and tested. |
|
Sweet! 💯 |
No description provided.