[GitHub] [hbase] apurtell commented on a change in pull request #3230: HBASE-25847 More DEBUG and TRACE level logging in CatalogJanitor and HbckChore
apurtell commented on a change in pull request #3230:
URL: https://github.com/apache/hbase/pull/3230#discussion_r626989255
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java
##
@@ -165,13 +168,19 @@ public int scan() throws IOException {
this.lastReport = scanForReport();
if (!this.lastReport.isEmpty()) {
LOG.warn(this.lastReport.toString());
+ } else {
+LOG.debug(this.lastReport.toString());
}
+
updateAssignmentManagerMetrics();
Map mergedRegions = this.lastReport.mergedRegions;
for (Map.Entry e : mergedRegions.entrySet()) {
if (this.services.isInMaintenanceMode()) {
// Stop cleaning if the master is in maintenance mode
+ if (LOG.isDebugEnabled()) {
+LOG.debug("In maintenence mode, not cleaning");
+ }
Review comment:
Ok both you and @bharathv mentioned this, will do.
--
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:
us...@infra.apache.org
[GitHub] [hbase] apurtell commented on a change in pull request #3230: HBASE-25847 More DEBUG and TRACE level logging in CatalogJanitor and HbckChore
apurtell commented on a change in pull request #3230:
URL: https://github.com/apache/hbase/pull/3230#discussion_r626988853
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java
##
@@ -165,13 +168,19 @@ public int scan() throws IOException {
this.lastReport = scanForReport();
if (!this.lastReport.isEmpty()) {
LOG.warn(this.lastReport.toString());
+ } else {
+LOG.debug(this.lastReport.toString());
Review comment:
We used to abide by that guidance but I don't see that done often in new
code.
--
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:
us...@infra.apache.org
[GitHub] [hbase] apurtell commented on a change in pull request #3230: HBASE-25847 More DEBUG and TRACE level logging in CatalogJanitor and HbckChore
apurtell commented on a change in pull request #3230:
URL: https://github.com/apache/hbase/pull/3230#discussion_r626219664
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java
##
@@ -317,11 +329,19 @@ static boolean cleanParent(MasterServices services,
RegionInfo parent, Result ro
LOG.debug("Deleting region " + parent.getShortNameToLog() + " because
daughters -- " +
daughterA + ", " + daughterB + " -- no longer hold references");
ProcedureExecutor pe =
services.getMasterProcedureExecutor();
- pe.submitProcedure(new GCRegionProcedure(pe.getEnvironment(), parent));
- // Remove from in-memory states
Review comment:
GCRegionProcedure does this. It is not needed here too. I think it
better to let the procedure do it. We expect the procedure to take care of such
details, so let's in fact do that.
Keeping this code around would be harmless, though.
--
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:
us...@infra.apache.org
[GitHub] [hbase] apurtell commented on a change in pull request #3230: HBASE-25847 More DEBUG and TRACE level logging in CatalogJanitor and HbckChore
apurtell commented on a change in pull request #3230:
URL: https://github.com/apache/hbase/pull/3230#discussion_r626219557
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java
##
@@ -252,14 +265,11 @@ private boolean cleanMergeRegion(final RegionInfo
mergedRegion, List
.stream().map(r ->
RegionInfo.getShortNameToLog(r)).collect(Collectors.joining(", ")),
mergedRegion);
ProcedureExecutor pe =
this.services.getMasterProcedureExecutor();
- pe.submitProcedure(
-new GCMultipleMergedRegionsProcedure(pe.getEnvironment(),
mergedRegion, parents));
- for (RegionInfo ri : parents) {
-// The above scheduled GCMultipleMergedRegionsProcedure does the below.
Review comment:
GCMultipleMergedRegionsProcedure does this. It is not needed here too. I
think it better to let the procedure do it. We expect the procedure to take
care of such details, so let's in fact do that.
Keeping this code around would be harmless, though.
--
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:
us...@infra.apache.org
[GitHub] [hbase] apurtell commented on a change in pull request #3230: HBASE-25847 More DEBUG and TRACE level logging in CatalogJanitor and HbckChore
apurtell commented on a change in pull request #3230:
URL: https://github.com/apache/hbase/pull/3230#discussion_r626219018
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HbckChore.java
##
@@ -223,15 +223,37 @@ private void loadRegionsFromInMemoryState() {
.isTableState(regionInfo.getTable(), TableState.State.DISABLED)) {
disabledTableRegions.add(regionInfo.getRegionNameAsString());
}
- if (regionInfo.isSplitParent()) {
+ if (regionState.isSplit()) {
Review comment:
Most of this patch is just more logging. This is a real bug fix, though.
Right now it just affects logging (numbers printed in a debug log line) but if
in the future code depends on `splitParentRegions` to be properly populated,
this will be needed.
--
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:
us...@infra.apache.org
