PHOENIX-5881 - Port PHOENIX-5645 (MaxLookbackAge) to 5.x #857
Conversation
|
Note that since HBase 2.1 and HBase 2.2 lack HBASE-24321, a lot of the logic is behind a 2.3 compatibility shim. |
|
@stoty - I'm curious about your opinion on how I'm using your compatibility projects to get version-specific coproc logic, plus version-specific feature flags. |
|
In 4.x we use org.apache.phoenix.compat.hbase.HbaseCompatCapabilities for the feature flags, It would be more consistent to move the feature flags there. I did not go through the code in detail, but it seems that extending BaseScannerRegionObserver from CompatBaseScannerRegionObserver would be a more elegant , and perhaps marginally faster solution than calling it explicitly. |
gjacoby126
force-pushed
the
PHOENIX-5881-v2
branch
from
983f3f5
to
69e0550
Compare
|
@stoty - introduced HbaseCompatCapabilities and subclassed from ComaptBaseScannerRegionObserver, as you suggested. |
|
|
||
| import org.apache.hadoop.conf.Configuration; | ||
|
|
||
| public class ScanInfoUtil { |
There was a problem hiding this comment.
This doesn't seem to be referenced anywhere.
| @@ -367,4 +374,16 @@ RegionScanner getWrappedScanner(final ObserverContext<RegionCoprocessorEnvironme | |||
| dataRegion, indexMaintainer, null, viewConstants, null, null, projector, ptr, useQualiferAsListIndex); | |||
| } | |||
|
|
|||
| @Override | |||
| public void preStoreScannerOpen(ObserverContext<RegionCoprocessorEnvironment> ctx, Store store, | |||
There was a problem hiding this comment.
This doesn't seem to do anything since the subclassing.
| } | ||
|
|
||
| //In HBase 2.1 and 2.2, a lookback query won't return any results if covered by a future delete | ||
| public static boolean isLookbackBeyondDeletesSupported() { return false; } |
There was a problem hiding this comment.
Nit: indent like above in both files?
| getConfiguration().getInt(CompatBaseScannerRegionObserver.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, | ||
| CompatBaseScannerRegionObserver.DEFAULT_PHOENIX_MAX_LOOKBACK_AGE); | ||
| long now = EnvironmentEdgeManager.currentTimeMillis(); | ||
| if (maxLookBackAge > 0 && now - maxLookBackAge * 1000L > scn){ |
There was a problem hiding this comment.
nit: would be good in understanding if s -> ms conversion happens outside of if condition.
There was a problem hiding this comment.
Not sure I follow? The configuration property is in seconds (to correspond with HBase TTL, which is also in seconds), but we convert to ms in the if-block to compare with the current timestamp.
There was a problem hiding this comment.
maxLookBackAge*1000 can be stored into a variable like done for ttl in other function.
There was a problem hiding this comment.
@swaroopak Switched to using an existing helper function to get the millisecond version of the max lookback config.
gjacoby126
force-pushed
the
PHOENIX-5881-v2
branch
from
bf40d12
to
1aa7d60
Compare
|
Thanks for the reviews, @stoty and @swaroopak . |
No description provided.