PHOENIX-5478 IndexTool mapper task should not timeout #603
Conversation
| public boolean next(List<Cell> results) throws IOException { | ||
| try { | ||
| byte[] uuidValue = ServerCacheClient.generateId(); | ||
| synchronized (this) { |
There was a problem hiding this comment.
Should this be synchronized(this.innerScanner)? It seems that doPostScannerOpen(), which calls RebuildIndices(), requires to synchronize the access to innerScanner object. By using "synchronized (this)", you synchronize the access to IndexRebuildRegionScanner object but it doesn't prevent this.innerScanner object being accessed in parallel.
There was a problem hiding this comment.
@BinShi-SecularBird, yes the previous code synchronized the access the innerScanner object. Actually, I am not sure why we need to synchronization here, how it would be different if you synchronize the access to the innerScanner vs this. Regardless, I agree with you that I should have preserved the previous code. I will revert this. Good catch.
| final Cell aggKeyValue = PhoenixKeyValueUtil.newKeyValue(UNGROUPED_AGG_ROW_KEY, SINGLE_COLUMN_FAMILY, | ||
| SINGLE_COLUMN, AGG_TIMESTAMP, rowCountBytes, 0, rowCountBytes.length); | ||
| results.add(aggKeyValue); | ||
| rowCount = 0; |
There was a problem hiding this comment.
It seems that rowCount needs not to be a member variable. Shall we change it to a local variable?
| throw e; | ||
| } finally { | ||
| region.closeRegionOperation(); | ||
| byte[] rowCountBytes = PLong.INSTANCE.toBytes(Long.valueOf(rowCount)); |
There was a problem hiding this comment.
Does rowCount here mean "the total count of rows successfully committed by commitBatchWithRetries"? If that's the case, we might have inconsistency issue -- suppose above loop called commitBatchWithRetries multiple times but the last one failed (thrown IOException) then rowCount here counted those uncommitted rows (MaxBatchSize - 1) -- is this expected?
There was a problem hiding this comment.
The row count is used to show the progress for the MR job. It is not really for correctness. However, if there is an exception, then the mapper task will get an exception and no row count will be returned to the mapper. The mapper task will start from the beginning to rebuild those rows again. When that happens, I expect the row count will be wrong because this mapper task will rebuild and double count for the rows that are rebuilt again. I will think about improving this, maybe not in this JIRA.
| if (put == null) { | ||
| put = new Put(CellUtil.cloneRow(cell)); | ||
| put.setAttribute(useProto ? PhoenixIndexCodec.INDEX_PROTO_MD : PhoenixIndexCodec.INDEX_MD, indexMetaData); | ||
| put.setAttribute(PhoenixIndexCodec.INDEX_UUID, uuidValue); |
There was a problem hiding this comment.
I know that you're just moving the code from elsewhere, but while we're here, could you please factor out the duplicate code between the if and else blocks to avoid duplication?
| @@ -208,7 +208,7 @@ | |||
| public static final long DEFAULT_INDEX_FAILURE_HANDLING_REBUILD_OVERLAP_BACKWARD_TIME = 1; // 1 ms | |||
| public static final long DEFAULT_INDEX_FAILURE_HANDLING_REBUILD_OVERLAP_FORWARD_TIME = 60000 * 3; // 3 mins | |||
| // 30 min rpc timeout * 5 tries, with 2100ms total pause time between retries | |||
| public static final long DEFAULT_INDEX_REBUILD_QUERY_TIMEOUT = 60000 * 60 * 24; // 24 hrs | |||
| public static final long DEFAULT_INDEX_REBUILD_QUERY_TIMEOUT = (5 * 30000 * 60) + 2100; | |||
There was a problem hiding this comment.
Where does this number come from?
There was a problem hiding this comment.
I reverted the change that I introduced in a previous JIRA for increasing timeouts.
There was a problem hiding this comment.
Also matches the the comment on line 210 right above.
No description provided.