[
https://issues.apache.org/jira/browse/PHOENIX-6388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17287898#comment-17287898
]
ASF GitHub Bot commented on PHOENIX-6388:
-----------------------------------------
virajjasani commented on a change in pull request #1146:
URL: https://github.com/apache/phoenix/pull/1146#discussion_r579782558
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/index/GlobalIndexChecker.java
##########
@@ -586,17 +595,28 @@ private boolean verifyRowAndRepairIfNecessary(List<Cell>
cellList) throws IOExce
long ts = cellList.get(0).getTimestamp();
cellList.clear();
+ long repairTime;
try {
repairIndexRows(rowKey, ts, cellList);
+ repairTime = EnvironmentEdgeManager.currentTimeMillis() -
repairStart;
metricsSource.incrementIndexRepairs(indexName);
metricsSource.updateUnverifiedIndexRowAge(indexName,
EnvironmentEdgeManager.currentTimeMillis() - ts);
metricsSource.updateIndexRepairTime(indexName,
EnvironmentEdgeManager.currentTimeMillis() -
repairStart);
+ if (shouldLog()) {
+ LOG.info(String.format("Index row repair on region %s
took %d ms.",
+ env.getRegionInfo().getRegionNameAsString(),
repairTime));
+ }
Review comment:
nit: `String.format()` is not required, we can use slf4j log placeholder
`{}`
```
LOG.info("Index row repair on region {} took {} ms.",
env.getRegionInfo().getRegionNameAsString(),
repairTime);
```
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/index/GlobalIndexChecker.java
##########
@@ -586,17 +595,28 @@ private boolean verifyRowAndRepairIfNecessary(List<Cell>
cellList) throws IOExce
long ts = cellList.get(0).getTimestamp();
cellList.clear();
+ long repairTime;
try {
repairIndexRows(rowKey, ts, cellList);
+ repairTime = EnvironmentEdgeManager.currentTimeMillis() -
repairStart;
metricsSource.incrementIndexRepairs(indexName);
metricsSource.updateUnverifiedIndexRowAge(indexName,
EnvironmentEdgeManager.currentTimeMillis() - ts);
metricsSource.updateIndexRepairTime(indexName,
EnvironmentEdgeManager.currentTimeMillis() -
repairStart);
+ if (shouldLog()) {
+ LOG.info(String.format("Index row repair on region %s
took %d ms.",
+ env.getRegionInfo().getRegionNameAsString(),
repairTime));
+ }
} catch (IOException e) {
+ repairTime = EnvironmentEdgeManager.currentTimeMillis() -
repairStart;
metricsSource.incrementIndexRepairFailures(indexName);
metricsSource.updateIndexRepairFailureTime(indexName,
EnvironmentEdgeManager.currentTimeMillis() -
repairStart);
+ if (shouldLog()) {
+ LOG.warn(String.format("Index row repair failure on
region %s took %d ms.",
+ env.getRegionInfo().getRegionNameAsString(),
repairTime));
+ }
Review comment:
Same here
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/index/GlobalIndexChecker.java
##########
@@ -101,6 +102,9 @@
*/
public class GlobalIndexChecker extends BaseScannerRegionObserver {
private static final Logger LOG =
LoggerFactory.getLogger(GlobalIndexChecker.class);
+ private static final String REPAIR_LOGGING_PERCENT_ATTRIB =
"phoenix.index.repair.logging.percent";
+ private static final double DEFAULT_REPAIR_LOGGING_PERCENT = 0.01;
Review comment:
As per `random.nextDouble() <= (loggingPercent / 100.0d)`, we will
compare `random.nextDouble()` with `0.01/100 = 0.0001` which seems way less. If
we can keep default value as `1` here, even `1/100` is going to be very less as
far as random distribution between 0 and 1 is concerned. Unless we never want
to log with default config value, i think we should change this to `1` (at
least some better probability of logging read repairs than `0.01` as default
value, which is almost never going to log)
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/index/GlobalIndexChecker.java
##########
@@ -586,17 +595,28 @@ private boolean verifyRowAndRepairIfNecessary(List<Cell>
cellList) throws IOExce
long ts = cellList.get(0).getTimestamp();
cellList.clear();
+ long repairTime;
try {
repairIndexRows(rowKey, ts, cellList);
+ repairTime = EnvironmentEdgeManager.currentTimeMillis() -
repairStart;
metricsSource.incrementIndexRepairs(indexName);
metricsSource.updateUnverifiedIndexRowAge(indexName,
EnvironmentEdgeManager.currentTimeMillis() - ts);
metricsSource.updateIndexRepairTime(indexName,
EnvironmentEdgeManager.currentTimeMillis() -
repairStart);
+ if (shouldLog()) {
+ LOG.info(String.format("Index row repair on region %s
took %d ms.",
+ env.getRegionInfo().getRegionNameAsString(),
repairTime));
+ }
Review comment:
This is good one, however regardless of `shouldLog()`, we might want to
consider logging at TRACE level always:
```
if (shouldLog()) {
LOG.info("Index row repair on region {} took {} ms.",
env.getRegionInfo().getRegionNameAsString(),
repairTime);
}
if (LOG.isTraceEnabled()) {
LOG.trace("Index row repair on region {} took {}
ms.",
env.getRegionInfo().getRegionNameAsString(),
repairTime);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> Add sampled logging for read repairs
> ------------------------------------
>
> Key: PHOENIX-6388
> URL: https://issues.apache.org/jira/browse/PHOENIX-6388
> Project: Phoenix
> Issue Type: Improvement
> Reporter: Xinyi Yan
> Assignee: Xinyi Yan
> Priority: Minor
> Fix For: 5.1.1, 4.16.1, 4.17.0
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
