Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6181 IndexRepairRegionScanner to verify and repair every glob… - #915
Conversation
stoty
commented
Oct 8, 2020
💔 -1 overall
This message was automatically generated. |
| batchSize += mutationList.size(); | ||
| if (batchSize >= maxBatchSize) { | ||
| ungroupedAggregateRegionObserver.checkForRegionClosing(); | ||
| region.batchMutate(indexUpdates.toArray(new Mutation[indexUpdates.size()]), | ||
| HConstants.NO_NONCE, HConstants.NO_NONCE); | ||
| batchSize = 0; | ||
| indexUpdates = new ArrayList<Mutation>(maxBatchSize); | ||
| } | ||
| } | ||
| if (batchSize > 0) { | ||
| ungroupedAggregateRegionObserver.checkForRegionClosing(); | ||
| region.batchMutate(indexUpdates.toArray(new Mutation[indexUpdates.size()]), | ||
| HConstants.NO_NONCE, HConstants.NO_NONCE); | ||
| } | ||
| batchSize = 0; | ||
| indexUpdates = new ArrayList<Mutation>(maxBatchSize); | ||
| for (Mutation mutation : indexRowsToBeDeleted) { | ||
| indexUpdates.add(mutation); | ||
| batchSize ++; | ||
| if (batchSize >= maxBatchSize) { | ||
| ungroupedAggregateRegionObserver.checkForRegionClosing(); | ||
| region.batchMutate(indexUpdates.toArray(new Mutation[indexUpdates.size()]), | ||
| HConstants.NO_NONCE, HConstants.NO_NONCE); | ||
| batchSize = 0; | ||
| indexUpdates = new ArrayList<Mutation>(maxBatchSize); | ||
| } | ||
| } | ||
| if (batchSize > 0) { | ||
| ungroupedAggregateRegionObserver.checkForRegionClosing(); | ||
| region.batchMutate(indexUpdates.toArray(new Mutation[indexUpdates.size()]), | ||
| HConstants.NO_NONCE, HConstants.NO_NONCE); |
There was a problem hiding this comment.
I feel the same logic of checking if the region is closed and then sending the mutations is duplicated in multiple places and we can move it to a function to reduce the code duplication.
There was a problem hiding this comment.
Question: Here we are calling region.batchMutate to update the index table but in IndexRebuildRegionScanner we call indexHTable.batch(). Why the difference ?
There was a problem hiding this comment.
I will reduce the code duplication here. When the index table is local (in the case of repair) we use the region API and when the index table is remote (in the case of rebuild), we use the table API.
| // All other types of rebuilds/verification should be incrementally performed if appropriate param is passed | ||
| byte[] lastVerifyTimeValue = scan.getAttribute(UngroupedAggregateRegionObserver.INDEX_RETRY_VERIFY); | ||
| Long lastVerifyTime = lastVerifyTimeValue == null ? 0 : Bytes.toLong(lastVerifyTimeValue); | ||
| if(indexRowKeyforReadRepair != null || lastVerifyTime == 0 || shouldVerifyCheckDone) { |
There was a problem hiding this comment.
Once this function is called, shouldVerifyCheckDone is set to true and from that point on, it will always be true.
Even if we want incremental, we will always do verify then why do we check lastVerifyTime? If lastVerifyTime is not 0, then shouldVerify check will not be set to true.
Can we have just 1 global variable to decide if we should verify than multiple to make this easier?
There was a problem hiding this comment.
lastVerifyTime is not a global variable. We can improve this method by checking shouldVerifyCheckDone at the entry of this method.
| @VisibleForTesting | ||
| public int setIndexTableTTL(int ttl) { | ||
| indexTableTTL = ttl; | ||
| return 0; |
There was a problem hiding this comment.
What is the purpose of always returning 0?
There was a problem hiding this comment.
This method is used only for the unit test. @swaroopak, can you answer the question?
| return; | ||
| } | ||
| if (!CellUtil.matchingValue(actualCell, expectedCell)) { | ||
| String errorMsg = "Not matching value (in iteration " + iteration + ") for " + Bytes.toString(family) + ":" + Bytes.toString(qualifier); |
There was a problem hiding this comment.
Family qualifier will not be super useful if we do 5928
There was a problem hiding this comment.
Did you mean 5923? Single cell format for indexes will be optional. Also, please note that there will be a separate cell for each family.
| batchSize += mutationList.size(); | ||
| if (batchSize >= maxBatchSize) { | ||
| ungroupedAggregateRegionObserver.checkForRegionClosing(); | ||
| region.batchMutate(indexUpdates.toArray(new Mutation[indexUpdates.size()]), | ||
| HConstants.NO_NONCE, HConstants.NO_NONCE); | ||
| batchSize = 0; | ||
| indexUpdates = new ArrayList<Mutation>(maxBatchSize); | ||
| } | ||
| } | ||
| if (batchSize > 0) { | ||
| ungroupedAggregateRegionObserver.checkForRegionClosing(); | ||
| region.batchMutate(indexUpdates.toArray(new Mutation[indexUpdates.size()]), | ||
| HConstants.NO_NONCE, HConstants.NO_NONCE); | ||
| } | ||
| batchSize = 0; | ||
| indexUpdates = new ArrayList<Mutation>(maxBatchSize); | ||
| for (Mutation mutation : indexRowsToBeDeleted) { | ||
| indexUpdates.add(mutation); | ||
| batchSize ++; | ||
| if (batchSize >= maxBatchSize) { | ||
| ungroupedAggregateRegionObserver.checkForRegionClosing(); | ||
| region.batchMutate(indexUpdates.toArray(new Mutation[indexUpdates.size()]), | ||
| HConstants.NO_NONCE, HConstants.NO_NONCE); | ||
| batchSize = 0; | ||
| indexUpdates = new ArrayList<Mutation>(maxBatchSize); | ||
| } | ||
| } | ||
| if (batchSize > 0) { | ||
| ungroupedAggregateRegionObserver.checkForRegionClosing(); | ||
| region.batchMutate(indexUpdates.toArray(new Mutation[indexUpdates.size()]), | ||
| HConstants.NO_NONCE, HConstants.NO_NONCE); |
| import org.slf4j.LoggerFactory; | ||
| import com.google.common.collect.Maps; | ||
There was a problem hiding this comment.
Most of the other classes have small descriptions of what they do. Let's add one here as well.
| return new IndexerRegionScanner(scanner, region, scan, env, this); | ||
| } else { | ||
| return new IndexRebuildRegionScanner(scanner, region, scan, env, this); | ||
| if (region.getTableDesc().hasCoprocessor(IndexRegionObserver.class.getCanonicalName())) { |
There was a problem hiding this comment.
Don't we have IndexRegionObserver for new design all the time? What is the case when the index doesn't have both Indexer and IndexRegionObserver?
There was a problem hiding this comment.
@gokceni IndexRegionObserver is only on the data table. Index table has neither Indexer nor IndexRegionObserver
There was a problem hiding this comment.
Yes you are right @tkhurana. I meant data table. Don't we remove Indexer and add IndexRegionObserver during upgrade?
There was a problem hiding this comment.
Is this an optimization for checking if this table is Index table @kadirozde rather than querying the PTable and looking at its type?
There was a problem hiding this comment.
We have not removed Indexer yet. Regarding the PTable comment, this is a server side code and in general we do not want to access the syscat on the server side for performance reasons mainly.
| } | ||
| return; | ||
| } | ||
| if (verifyType == IndexTool.IndexVerifyType.AFTER) { |
There was a problem hiding this comment.
The AFTER option will not remove the extra verified rows in the index table. Same with the NONE option. Does it make sense to have these options when using the index table as the source ?
There was a problem hiding this comment.
I will leave this decision to be made within IndexTool. I think we can still allow these options and state in the help text for IndexTook that they do not remove the stale index rows.
stoty
commented
Oct 15, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 16, 2020
💔 -1 overall
This message was automatically generated. |
…al index row