add ENABLE_DTX_INGEST_BATCH option #799
Conversation
…e unlogged batch statements for Ingest path
| @@ -38,15 +37,13 @@ | |||
| private static final String TENANT2 = "987654"; | |||
| private static final String TENANT3 = "123789"; | |||
|
|
|||
| protected LocatorIO locatorIO = new DLocatorIO(); | |||
| protected DelayedLocatorIO delayedLocatorIO = new DDelayedLocatorIO(); | |||
| protected DLocatorIO locatorIO = new DLocatorIO(); | |||
There was a problem hiding this comment.
Not a big fan of this change. Cant you cast locatorIO to DLocatorIO where ever you need to call getBoundStatementForLocator? Now that I think about it, I dont know which one is less bad. Nevermind then.
There was a problem hiding this comment.
I think this is ok. Only the DBasicMetricsRW class needs locatorIO and DelayedLocatorIO for its constructor. The ABasicMetricsRW class doesn't need those IO classes. (Don't ask me why, tho). 😬 Since DBasicMetricsRW is Datastax specific, it is ok to pass in Datastax specific IO classes to its constructor.
| * @param metric | ||
| * @return | ||
| */ | ||
| protected boolean isDelayed(IMetric metric) { |
| BatchStatement batch = new BatchStatement(BatchStatement.Type.UNLOGGED); | ||
|
|
||
| for (Locator locator : map.keySet()) { | ||
| for (IMetric metric : map.get(locator)) { |
There was a problem hiding this comment.
Should we have Instrumentation call here?
There was a problem hiding this comment.
Good catch. I will add Instrumentation, similar to insertMetricsIndividually()
| Object metricValue = metric.getMetricValue(); | ||
| if (!(metricValue instanceof BluefloodSetRollup)) { | ||
| throw new InvalidDataException( | ||
| String.format("getBoundStatementForMetric(locator=%s, granularity=%s): metric value %s is not type BfSetRollup", |
There was a problem hiding this comment.
Should we write the actual type BluefloodSetRollup instead of BfSetRollup in the exception message? This comment applies to other places like counter, gauge etc as well.
There was a problem hiding this comment.
I will change that.
| * @param granularity | ||
| * @return | ||
| */ | ||
| protected abstract BoundStatement getBoundStatementForMetric(IMetric metric, Granularity granularity); |
There was a problem hiding this comment.
Cant we have some default implementation here? Since each sub classes seem to have similar implementation?
There was a problem hiding this comment.
I can't think of a useful way to provide default implementation here. If you look at the implementation of this method in the sub classes, each line requires knowledge of what object (BluefloodSetRollup, BasicRollup, etc) you are operating on. I briefly thought of using generic types but decided it's not worth it.
Do you have specific suggestions?
There was a problem hiding this comment.
Each of the subclasses override an abstract method called getRollupType() which returns (BluefloodSetRollup, BasicRollup, etc). The getBoundStatementForMetric calls getRollupType() to get the type. I dont know if that covers all the issues.
If its too much hassle, I am fine with leaving the way it is now.
There was a problem hiding this comment.
But we need to use this type in a cast, like this:
serDes.serialize( (BluefloodSetRollup) metricValue ),
| private IOContainer(DriverType driver, boolean isRecordingDelayedMetrics) { | ||
| private IOContainer(DriverType driver, | ||
| boolean isRecordingDelayedMetrics, | ||
| boolean isDtxIngestBatchEnabled) { |
There was a problem hiding this comment.
This change actually bothers me more than the change from LocatorIO to DLocatorIO in DBasicMetricsRWIntegrationTest. But I can't think of a better way (but still simple) way to do it.
This PR adds a new configuration option: ENABLE_DTX_INGEST_BATCH. When set to true, blueflood will use unlogged batch statements during Ingest, if CASSANDRA_DRIVER is set to datastax. The default behavior for this option is false: i.e: use individual asynch INSERT statement (not unlogged batch).
If this option is set to true, both Basic metrics and Preaggregated metrics will use unlogged batches.