You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2021/02/21 10:02:50 UTC
[GitHub] [phoenix] virajjasani commented on a change in pull request #1146: PHOENIX-6388 Add sampled logging for read repairs
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:
users@infra.apache.org