Issue #97, #98: Aggregate job info per day and per week as part of JobPr... #113
Conversation
… as part of JobProcessing Step in the ETL. Also enable a rest api to fetch this aggregated app info, use check and put to store number of runs, cost and queues,
|
Previous pull request for the same: #99 |
|
This pull request also updates columns in the raw table that specify the status of aggregation for daily and weekly for that job. If the job has already been aggregated, aggregation will not be re-attempted unless the re-aggregate flag is turned on. This helps avoid inadvertent re-aggregation, since aggregation is not idempotent. Also, it implements Check and Put methods for queue list, job cost and number of runs in the info column family with retries. The intention here is to ensure we update these columns carefully since multiple tasks/jobs may be updating these columns for the same app for that day or that week. More details in the code in comments |
| # the s column family has a TTL of 30 days, it's used as a scratch col family | ||
| # it stores the run ids that are seen for that day | ||
| # we assume that a flow will not run for more than 30 days, hence it's fine to "expire" that data | ||
| create 'hraven_agg_daily', {NAME => 'i', COMPRESSION => 'LZO', BLOOMFILTER => 'ROWCOL'}, |
There was a problem hiding this comment.
Any reason you used "hraven_..." as the names of these new tables? Doesn't look like that is the naming convention followed by other tables?
There was a problem hiding this comment.
I think using an hraven prefix for table names is the better way ahead considering that the hbase datastore can contain tables from other applications as well.
For existing tables, renaming them now may be a bit more complex since hbase does not have a rename command. The recommended way has a few steps, as listed here http://hbase.apache.org/book/table.rename.html
We would need to disable the table and use clone snapshot to do it:
hbase shell> disable 'tableName'
hbase shell> snapshot 'tableName', 'tableSnapshot'
hbase shell> clone_snapshot 'tableSnapshot', 'newTableName'
hbase shell> delete_snapshot 'tableSnapshot'
hbase shell> drop 'tableName'
There was a problem hiding this comment.
I modified the table names to be prefixed with job_history so that it is more consistent with other hraven tables
…m navigable map of hbase results
|
Changes Unknown when pulling ba338ec on vrushalic:etl_aggregate_2 into * on twitter:master*. |
| * in daily or weekly aggregation table | ||
| * @param {@link JobDetails} | ||
| */ | ||
| public Boolean aggregateJobDetails(JobDetails jobDetails, |
There was a problem hiding this comment.
Is returning the object type Boolean necessary? I think it is just fine to use the primitive type (boolean). Using the object would unnecessarily cause boxing and unboxing and sometimes cause subtle bugs.
| newAppsKeys = createNewAppKeysFromResults(scan, startTime, endTime, limit); | ||
| } catch (IOException e) { | ||
| LOG.error("Caught exception while trying to scan, returning empty list of flows: " | ||
| + e.toString()); |
There was a problem hiding this comment.
Although it is existing code, it makes me wonder. If this threw an exception, can you proceed to the next? I would think newAppsKeys is null, and you'd get a NullPointerException in line 117.
There was a problem hiding this comment.
I looked through the code again, it looks like the function createNewAppKeysFromResults will always return a non-null list (either empty or populated). And I also see some "Long" objects there which can be changed to "long"
…ong, logging exceptions correctly etc)
|
Changes Unknown when pulling 71d4a5b on vrushalic:etl_aggregate_2 into * on twitter:master*. |
|
Changes Unknown when pulling 71d4a5b on vrushalic:etl_aggregate_2 into * on twitter:master*. |
|
Changes Unknown when pulling 71d4a5b on vrushalic:etl_aggregate_2 into * on twitter:master*. |
| * name of the flag that determines whether or not re-aggregate | ||
| * (overrides aggregation status in raw table for that job) | ||
| */ | ||
| public static String RE_AGGREGATION_FLAG_NAME = "reaggregate"; |
|
LGTM. Thanks for your patience @vrushalic! It seems like the travis build is stuck for some reason. It might be good to get a green build. We can merge this once @jrottinghuis gives his +1. |
|
Yes, that build isn't even starting up. I tried cancelling it and restarted it. I will keep an eye on this. If nothing changes by today evening, I will make a simple checkin (like a comment update or something) to trigger another build on this branch. |
|
Changes Unknown when pulling 9661ad2 on vrushalic:etl_aggregate_2 into * on twitter:master*. |
|
The travis CI build has passed and is green |
...ocessing Step in the ETL. Also enable a rest api to fetch this aggregated app info, use check and put to store number of runs, cost and queues,