[
https://issues.apache.org/jira/browse/PHOENIX-5494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16974811#comment-16974811
]
chenglei edited comment on PHOENIX-5494 at 11/15/19 10:22 AM:
--------------------------------------------------------------
[~larsh], sorry for later response. I have made a patch last two days using
{{SkipScanFilter}} as you suggested to solve my problem in my production
environment(which is hbase1.4.10).
After reading [~kozdemir] 's patch, there are some suggestions from my patch
viewpoint which I think we can do better:
1. Scan.startRow and Scan.endRow should be set to improve the scanner
performance in additional to just using the MultiRowRangeFilter(In fact we can
also use SkipScanFilter here):
{code:java}
s.setFilter(new MultiRowRangeFilter(ranges));
s.setTimeRange(0, ts);
Region region = this.env.getRegion();
try (RegionScanner scanner = region.getScanner(s)) {
{code}
2. In additional to normal updates, we can also using the this optimization for
replaying write.
3. {{LocalTable}} is global region-scope and is stateless, so if we add a
member variable {{LocalTable.results}} to make it stateful, there may be many
concurrent issues. I think we would better create a new class(for example
CachedLocalTable) to hold the results we pre-scannered and make the object as a
local variable, passing it to downstream {{IndexBuilder.getIndexUpdate}} to
avoid concurrent issues and lock contention completely, and
{{IndexBuildManager.removeRowStates}} is no need.
such as:
IndexBuildManager.getIndexUpdates
{code:java}
public Collection<Pair<Pair<Mutation, byte[]>, byte[]>> getIndexUpdates(
MiniBatchOperationInProgress<Mutation> miniBatchOp,
Collection<? extends Mutation> mutations,
IndexMetaData indexMetaData,
long now) throws Throwable {
// notify the delegate that we have started processing a batch
this.delegate.batchStarted(miniBatchOp, indexMetaData);
CachedLocalTable cachedLocalTable =
preScanAllRequiredRows(
mutations,
(PhoenixIndexMetaData)indexMetaData,
now,
this.regionCoprocessorEnvironment);
// Avoid the Object overhead of the executor when it's not actually
parallelizing anything.
ArrayList<Pair<Pair<Mutation, byte[]>, byte[]>> results = new
ArrayList<>(mutations.size());
for (Mutation m : mutations) {
Collection<Pair<Mutation, byte[]>> updates = delegate.getIndexUpdate(m,
indexMetaData, cachedLocalTable);
if (PhoenixIndexMetaData.isIndexRebuild(m.getAttributesMap())) {
for (Pair<Mutation, byte[]> update : updates) {
update.getFirst().setAttribute(BaseScannerRegionObserver.REPLAY_WRITES,
BaseScannerRegionObserver.REPLAY_INDEX_REBUILD_WRITES);
}
}
{code}
NonTxIndexBuilder.getIndexUpdate
{code:java}
@Override
public Collection<Pair<Mutation, byte[]>> getIndexUpdate(Mutation mutation,
IndexMetaData indexMetaData, LocalHBaseState localHBaseState) throws
IOException {
// create a state manager, so we can manage each batch
LocalTableState state = new LocalTableState(localHBaseState, mutation);
// build the index updates for each group
IndexUpdateManager manager = new IndexUpdateManager(indexMetaData);
batchMutationAndAddUpdates(manager, state, mutation, indexMetaData);
if (LOGGER.isTraceEnabled()) {
LOGGER.trace("Found index updates for Mutation: " + mutation + "\n"
+ manager);
}
return manager.toMap();
}
{code}
4. I think making the modifications sealed to {{IndexBuildManager}} is better,
{{IndexRegionObserver}} is no need to know the pre-scan, so this optimization
can also be used by another classes such as the old {{Indexer}} coprocessor.
5. In {{LocalTable.getCurrentRowState}}, Why we should do the scan again if we
can not find the result in the HashMap? I think it is unnecessary.
I uploaded my patch on 4.x-HBase-1.4 which passed all tests including
ConcurrentMutationsIT, for [~kozdemir] to reference or to see whether there are
somethings could merge into his patch.
was (Author: comnetwork):
[~larsh], sorry for later response. I have made a patch last two days using
{{SkipScanFilter}} you suggested to solve my problem in my production
environment(which is hbase1.4.10).
After reading [~kozdemir] 's patch, there are some suggestions from my patch
viewpoint which I think we can do better:
1. Scan.startRow and Scan.endRow should be set to improve the scanner
performance in additional to just using the MultiRowRangeFilter(In fact we can
also use SkipScanFilter here):
{code:java}
s.setFilter(new MultiRowRangeFilter(ranges));
s.setTimeRange(0, ts);
Region region = this.env.getRegion();
try (RegionScanner scanner = region.getScanner(s)) {
{code}
2. In additional to normal updates, we can also using the this optimization for
replaying write.
3. {{LocalTable}} is global region-scope, so if we add a member variable
{{LocalTable.results}} to make it stateful, there may be many concurrent
issues. I think we would better create a new class(for example
CachedLocalTable) to hold the results we pre-scannered and make the object as a
local variable, passing it to downstream {{IndexBuilder.getIndexUpdate}} to
avoid concurrent issues and lock contention completely, and
{{IndexBuildManager.removeRowStates}} is no need.
such as:
IndexBuildManager.getIndexUpdates
{code:java}
public Collection<Pair<Pair<Mutation, byte[]>, byte[]>> getIndexUpdates(
MiniBatchOperationInProgress<Mutation> miniBatchOp,
Collection<? extends Mutation> mutations,
IndexMetaData indexMetaData,
long now) throws Throwable {
// notify the delegate that we have started processing a batch
this.delegate.batchStarted(miniBatchOp, indexMetaData);
CachedLocalTable cachedLocalTable =
preScanAllRequiredRows(
mutations,
(PhoenixIndexMetaData)indexMetaData,
now,
this.regionCoprocessorEnvironment);
// Avoid the Object overhead of the executor when it's not actually
parallelizing anything.
ArrayList<Pair<Pair<Mutation, byte[]>, byte[]>> results = new
ArrayList<>(mutations.size());
for (Mutation m : mutations) {
Collection<Pair<Mutation, byte[]>> updates = delegate.getIndexUpdate(m,
indexMetaData, cachedLocalTable);
if (PhoenixIndexMetaData.isIndexRebuild(m.getAttributesMap())) {
for (Pair<Mutation, byte[]> update : updates) {
update.getFirst().setAttribute(BaseScannerRegionObserver.REPLAY_WRITES,
BaseScannerRegionObserver.REPLAY_INDEX_REBUILD_WRITES);
}
}
{code}
NonTxIndexBuilder.getIndexUpdate
{code:java}
@Override
public Collection<Pair<Mutation, byte[]>> getIndexUpdate(Mutation mutation,
IndexMetaData indexMetaData, LocalHBaseState localHBaseState) throws
IOException {
// create a state manager, so we can manage each batch
LocalTableState state = new LocalTableState(localHBaseState, mutation);
// build the index updates for each group
IndexUpdateManager manager = new IndexUpdateManager(indexMetaData);
batchMutationAndAddUpdates(manager, state, mutation, indexMetaData);
if (LOGGER.isTraceEnabled()) {
LOGGER.trace("Found index updates for Mutation: " + mutation + "\n"
+ manager);
}
return manager.toMap();
}
{code}
4. I think making the modifications sealed to {{IndexBuildManager}} is better,
{{IndexRegionObserver}} is no need to know the pre-scan, so this optimization
can also be used by another classes such as the old {{Indexer}} coprocessor.
I uploaded my patch on 4.x-HBase-1.4 which passed all tests including
ConcurrentMutationsIT, for [~kozdemir] to reference or to see whether there are
somethings could merge into his patch.
> Batched, mutable Index updates are unnecessarily run one-by-one
> ---------------------------------------------------------------
>
> Key: PHOENIX-5494
> URL: https://issues.apache.org/jira/browse/PHOENIX-5494
> Project: Phoenix
> Issue Type: Improvement
> Reporter: Lars Hofhansl
> Assignee: Kadir OZDEMIR
> Priority: Major
> Labels: performance
> Attachments: 5494-4.x-HBase-1.5.txt,
> PHOENIX-5494-4.x-HBase-1.4.patch, PHOENIX-5494.master.001.patch,
> PHOENIX-5494.master.002.patch, PHOENIX-5494.master.003.patch,
> Screenshot_20191110_160243.png, Screenshot_20191110_160351.png,
> Screenshot_20191110_161453.png
>
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> I just noticed that index updates on mutable tables retrieve their deletes
> (to invalidate the old index entry) one-by-one.
> For batches, this can be *the* major time spent during an index update. The
> cost is mostly incured by the repeated setup (and seeking) of the new region
> scanner (for each row).
> We can instead do a skip scan and get all updates in a single scan per region.
> (Logically that is simple, but it will require some refactoring)
> I won't be getting to this, but recording it here in case someone feels
> inclined.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
