stoty commented on a change in pull request #1146:
URL: https://github.com/apache/phoenix/pull/1146#discussion_r580022034
##########
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:
On one hand, logging every event with trace would make sense from an
operational standpoint, as it can theoretically be enabled without restarting
the RS (I THINK that LOG4j will pick logging config file changes during runtime)
On the other hand I feel that having a statistic logging at INFO level, but
also logging everything at TRACE level would go against the 'do not
surprise/confuse the user' principle. AFAIK we have other statistic logging
events, which cover similar cases, but we don't override the percentage setting
like this.
IMO **REPAIR_LOGGING_PERCENT_ATTRIB sets the probability for logging the
event at INFO, but we're logging every event at TRACE anyway** is not intuitive
behaviour, nor is logging the same event twice at different logging levels.
Unfiltered trace logging in production systems in unsustainable, and if
someone wants to do targeted log index rebuilds, he can just use the percentage
setting (which is easier to get right than logging config, even if it does
require an RS restart to take effect)
----------------------------------------------------------------
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]
