[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r364951164
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -244,4 +301,47 @@ static boolean isOverlap(RegionInfo ri, Pair pair) {
}
return ri.isOverlap(pair.getFirst()) || ri.isOverlap(pair.getSecond());
}
+
+ /**
+ * A union over {@link L} and {@link R}.
+ */
+ private static class Either {
+private final L left;
+private final R right;
+
+public static Either ofLeft(L left) {
+ return new Either<>(left, null);
+}
+
+public static Either ofRight(R right) {
+ return new Either<>(null, right);
+}
+
+Either(L left, R right) {
+ this.left = left;
+ this.right = right;
+}
+
+public boolean hasLeft() {
+ return left != null;
+}
+
+public L getLeft() {
+ if (!hasLeft()) {
+throw new IllegalStateException("Either contains no left.");
+ }
+ return left;
+}
+
+public boolean hasRight() {
+ return right != null;
+}
+
+public R getRight() {
+ if (!hasRight()) {
+throw new IllegalStateException("Either contains no right.");
+ }
+ return right;
+}
+ }
}
Review comment:
Fancy. As long as unit tests still pass (I think the unit test was
pretty involved IIRC)
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r364949411
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +84,133 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
return;
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
-}
+
+LOG.info("Identified {} region holes to fix. Detailed fixup progress
logged at DEBUG.",
+ holes.size());
+
+final List newRegionInfos = createRegionInfosForHoles(holes);
+final List newMetaEntries = createMetaEntries(masterServices,
newRegionInfos);
+final TransitRegionStateProcedure[] assignProcedures = masterServices
+ .getAssignmentManager()
+ .createRoundRobinAssignProcedures(newMetaEntries);
+
+
masterServices.getMasterProcedureExecutor().submitProcedures(assignProcedures);
+LOG.info(
Review comment:
Superfluous? Doesn't procedure spew out logs itself?
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r364948800
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +84,133 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
Review comment:
Did I say it before that CJ will report this every time it runs No
objection to letting operator know whats up but would rather this part of a
single reporting line for CJ instead of a log per task done by the CJ.
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r364949659
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +84,133 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
return;
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
-}
+
+LOG.info("Identified {} region holes to fix. Detailed fixup progress
logged at DEBUG.",
+ holes.size());
+
+final List newRegionInfos = createRegionInfosForHoles(holes);
+final List newMetaEntries = createMetaEntries(masterServices,
newRegionInfos);
+final TransitRegionStateProcedure[] assignProcedures = masterServices
+ .getAssignmentManager()
+ .createRoundRobinAssignProcedures(newMetaEntries);
+
+
masterServices.getMasterProcedureExecutor().submitProcedures(assignProcedures);
+LOG.info(
Review comment:
Or maybe this is ok. Check logs. If not too much, then ignore this remark.
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r364949013
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +84,133 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
return;
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
-}
+
+LOG.info("Identified {} region holes to fix. Detailed fixup progress
logged at DEBUG.",
Review comment:
This log is ok though because working on a prob (no log if no prob I'd
suggest)
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r356801303
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +87,176 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
return;
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
-}
+
+LOG.info("Identified {} region holes to fix.", holes.size());
+
+final List newRegionInfos = mapHolesToRegionInfos(holes);
+final List newRegionDirectories =
+ createRegionDirectories(masterServices, newRegionInfos);
+final List newMetaEntries = createMetaEntries(masterServices,
newRegionDirectories);
+final List scheduledProcedures =
+
masterServices.getAssignmentManager().createAssignProceduresBestEffort(newMetaEntries);
+LOG.info(
+ "Scheduled {}/{} new regions for assignment.",
scheduledProcedures.size(), holes.size());
}
/**
- * @return Calculated RegionInfo that covers the hole hole
+ * Create a new {@link RegionInfo} corresponding to each provided "hole"
pair.
*/
- private RegionInfo getHoleCover(Pair hole) {
-RegionInfo holeCover = null;
-RegionInfo left = hole.getFirst();
-RegionInfo right = hole.getSecond();
+ private static List mapHolesToRegionInfos(
Review comment:
Latter makes more sense to me.
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r357866946
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -83,77 +85,193 @@ void fix() throws IOException {
}
/**
- * If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
- * Does not assign.
+ * For each hole identified in {@code report}, generates a corresponding
region, adds it to the
+ * filesystem, adds it to hbase:meta, and requests assignment. All actions
are best-effort.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ TransitRegionStateProcedure[] fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
- return;
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
+ return new TransitRegionStateProcedure[0];
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
+
+LOG.info("Identified {} region holes to fix.", holes.size());
+
+final List newRegionInfos = mapHolesToRegionInfos(holes);
+final List newRegionDirectories =
+ createRegionDirectories(masterServices, newRegionInfos);
+final List newMetaEntries = createMetaEntries(masterServices,
newRegionDirectories);
+
+// this call uses the bulk method `createAssignProcedures` instead of
one-at-a-time method
+// `assign`. The bulk method makes no assertions on existing region state,
while `assign` does.
+// Probably we would like to assertions to be run, but since we're just
creating these regions,
+// we assume those preconditions to be satisfied (regionState =
State.CLOSED).
Review comment:
Good
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r357866250
##
File path:
hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/ProcedureTestingUtility.java
##
@@ -275,6 +276,12 @@ public static LoadCounter
storeRestartAndAssert(ProcedureStore procStore, long m
}
}
+ public static void waitProcedures(ProcedureExecutor
procedureExecutor, Procedure... procs) {
+if (procs == null) { return; }
Review comment:
This is an interesting way of doing one-line. I think I might like it.
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r356800731
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +87,176 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
Review comment:
ok
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r357866822
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -83,77 +85,193 @@ void fix() throws IOException {
}
/**
- * If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
- * Does not assign.
+ * For each hole identified in {@code report}, generates a corresponding
region, adds it to the
+ * filesystem, adds it to hbase:meta, and requests assignment. All actions
are best-effort.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ TransitRegionStateProcedure[] fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
- return;
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
+ return new TransitRegionStateProcedure[0];
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
+
+LOG.info("Identified {} region holes to fix.", holes.size());
+
+final List newRegionInfos = mapHolesToRegionInfos(holes);
Review comment:
Were you going to change the name of this method?
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r357869090
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -83,77 +85,193 @@ void fix() throws IOException {
}
/**
- * If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
- * Does not assign.
+ * For each hole identified in {@code report}, generates a corresponding
region, adds it to the
+ * filesystem, adds it to hbase:meta, and requests assignment. All actions
are best-effort.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ TransitRegionStateProcedure[] fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
- return;
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
+ return new TransitRegionStateProcedure[0];
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
+
+LOG.info("Identified {} region holes to fix.", holes.size());
+
+final List newRegionInfos = mapHolesToRegionInfos(holes);
+final List newRegionDirectories =
+ createRegionDirectories(masterServices, newRegionInfos);
+final List newMetaEntries = createMetaEntries(masterServices,
newRegionDirectories);
+
+// this call uses the bulk method `createAssignProcedures` instead of
one-at-a-time method
+// `assign`. The bulk method makes no assertions on existing region state,
while `assign` does.
+// Probably we would like to assertions to be run, but since we're just
creating these regions,
+// we assume those preconditions to be satisfied (regionState =
State.CLOSED).
+final TransitRegionStateProcedure[] scheduledProcedures =
+
masterServices.getAssignmentManager().createAssignProcedures(newMetaEntries);
+
masterServices.getMasterProcedureExecutor().submitProcedures(scheduledProcedures);
+LOG.info(
+ "Submitted {}/{} new regions for assignment.",
scheduledProcedures.length, holes.size());
+if (LOG.isDebugEnabled()) {
+ final String procIds = Arrays.stream(scheduledProcedures)
+.mapToLong(TransitRegionStateProcedure::getProcId)
+.mapToObj(Long::toString)
+.collect(Collectors.joining(", "));
+ LOG.debug("Submitted assignment procIds: {}", procIds);
Review comment:
Do we log all procedure submits already?
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r357869010
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -83,77 +85,193 @@ void fix() throws IOException {
}
/**
- * If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
- * Does not assign.
+ * For each hole identified in {@code report}, generates a corresponding
region, adds it to the
+ * filesystem, adds it to hbase:meta, and requests assignment. All actions
are best-effort.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ TransitRegionStateProcedure[] fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
- return;
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
+ return new TransitRegionStateProcedure[0];
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
+
+LOG.info("Identified {} region holes to fix.", holes.size());
+
+final List newRegionInfos = mapHolesToRegionInfos(holes);
+final List newRegionDirectories =
+ createRegionDirectories(masterServices, newRegionInfos);
+final List newMetaEntries = createMetaEntries(masterServices,
newRegionDirectories);
+
+// this call uses the bulk method `createAssignProcedures` instead of
one-at-a-time method
+// `assign`. The bulk method makes no assertions on existing region state,
while `assign` does.
+// Probably we would like to assertions to be run, but since we're just
creating these regions,
+// we assume those preconditions to be satisfied (regionState =
State.CLOSED).
+final TransitRegionStateProcedure[] scheduledProcedures =
+
masterServices.getAssignmentManager().createAssignProcedures(newMetaEntries);
+
masterServices.getMasterProcedureExecutor().submitProcedures(scheduledProcedures);
+LOG.info(
+ "Submitted {}/{} new regions for assignment.",
scheduledProcedures.length, holes.size());
+if (LOG.isDebugEnabled()) {
Review comment:
nit: either INFO or the DEBUG but not both.
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r356801045
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +87,176 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
return;
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
-}
+
+LOG.info("Identified {} region holes to fix.", holes.size());
Review comment:
Ok. There is a plan.
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r356801604
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -244,4 +347,47 @@ static boolean isOverlap(RegionInfo ri, Pair pair) {
}
return ri.isOverlap(pair.getFirst()) || ri.isOverlap(pair.getSecond());
}
+
+ /**
+ * A union over {@link L} and {@link R}.
+ */
+ private static class Either {
Review comment:
Makes sense.
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r357866768
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -83,77 +85,193 @@ void fix() throws IOException {
}
/**
- * If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
- * Does not assign.
+ * For each hole identified in {@code report}, generates a corresponding
region, adds it to the
+ * filesystem, adds it to hbase:meta, and requests assignment. All actions
are best-effort.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ TransitRegionStateProcedure[] fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
- return;
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
+ return new TransitRegionStateProcedure[0];
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
+
+LOG.info("Identified {} region holes to fix.", holes.size());
+
+final List newRegionInfos = mapHolesToRegionInfos(holes);
+final List newRegionDirectories =
+ createRegionDirectories(masterServices, newRegionInfos);
Review comment:
You don't have to create region directories... They are auto-created.
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r355680221
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +87,176 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
Review comment:
Why name the class in the log message? Isn't it present before the above log
txt in the logs already?
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r355133022
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestAMAssignWithRandExec.java
##
@@ -44,10 +46,15 @@ public void testAssignWithRandExec() throws Exception {
rsDispatcher.setMockRsExecutor(new RandRsExecutor());
// Loop a bunch of times so we hit various combos of exceptions.
+int submitCount = 0;
for (int i = 0; i < 10; i++) {
LOG.info("ROUND=" + i);
TransitRegionStateProcedure proc = createAssignProcedure(hri);
- waitOnFuture(submitProcedure(proc));
+ if (proc != null) {
+waitOnFuture(submitProcedure(proc));
+submitCount++;
+ }
}
+assertNotEquals("failed to create any procedures", 0, submitCount);
Review comment:
Good
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing URL: https://github.com/apache/hbase/pull/917#discussion_r355133148 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TruncateTableProcedure.java ## @@ -139,7 +140,10 @@ protected Flow executeFromState(final MasterProcedureEnv env, TruncateTableState break; case TRUNCATE_TABLE_ASSIGN_REGIONS: CreateTableProcedure.setEnablingState(env, getTableName()); - addChildProcedure(env.getAssignmentManager().createRoundRobinAssignProcedures(regions)); + final TransitRegionStateProcedure[] assignments = env.getAssignmentManager() +.createRoundRobinAssignProcedures(regions) +.toArray(new TransitRegionStateProcedure[0]); + addChildProcedure(assignments); Review comment: Old way was better? 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 With regards, Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r355684692
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -244,4 +347,47 @@ static boolean isOverlap(RegionInfo ri, Pair pair) {
}
return ri.isOverlap(pair.getFirst()) || ri.isOverlap(pair.getSecond());
}
+
+ /**
+ * A union over {@link L} and {@link R}.
+ */
+ private static class Either {
Review comment:
Nice.
Belongs in RegionInfo? Or you think only for use by fixer?
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing URL: https://github.com/apache/hbase/pull/917#discussion_r355132809 ## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestAssignmentManagerBase.java ## @@ -26,7 +27,10 @@ import java.io.UncheckedIOException; import java.net.SocketTimeoutException; import java.util.Arrays; +import java.util.Collections; +import java.util.List; import java.util.NavigableMap; +import java.util.Optional; Review comment: Unused imports? 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 With regards, Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r355683606
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +87,176 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
return;
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
-}
+
+LOG.info("Identified {} region holes to fix.", holes.size());
+
+final List newRegionInfos = mapHolesToRegionInfos(holes);
+final List newRegionDirectories =
+ createRegionDirectories(masterServices, newRegionInfos);
+final List newMetaEntries = createMetaEntries(masterServices,
newRegionDirectories);
+final List scheduledProcedures =
+
masterServices.getAssignmentManager().createAssignProceduresBestEffort(newMetaEntries);
+LOG.info(
+ "Scheduled {}/{} new regions for assignment.",
scheduledProcedures.size(), holes.size());
}
/**
- * @return Calculated RegionInfo that covers the hole hole
+ * Create a new {@link RegionInfo} corresponding to each provided "hole"
pair.
*/
- private RegionInfo getHoleCover(Pair hole) {
-RegionInfo holeCover = null;
-RegionInfo left = hole.getFirst();
-RegionInfo right = hole.getSecond();
+ private static List mapHolesToRegionInfos(
+final List> holes) {
+final List newRegionInfos = holes.stream()
+ .map(MetaFixer::getHoleCover)
+ .filter(Optional::isPresent)
+ .map(Optional::get)
+ .collect(Collectors.toList());
+LOG.debug("Constructed {}/{} RegionInfo descriptors corresponding to
identified holes.",
+ newRegionInfos.size(), holes.size());
+return newRegionInfos;
+ }
+
+ /**
+ * @return Attempts to calculate a new {@link RegionInfo} that covers the
region range described
+ * in {@code hole}.
+ */
+ private static Optional getHoleCover(Pair hole) {
+final RegionInfo left = hole.getFirst();
+final RegionInfo right = hole.getSecond();
+
if (left.getTable().equals(right.getTable())) {
// Simple case.
if (Bytes.compareTo(left.getEndKey(), right.getStartKey()) >= 0) {
-LOG.warn("Skipping hole fix; left-side endKey is not less than
right-side startKey; " +
-"left=<{}>, right=<{}>", left, right);
-return holeCover;
- }
- holeCover = buildRegionInfo(left.getTable(), left.getEndKey(),
right.getStartKey());
-} else {
- boolean leftUndefined = left.equals(RegionInfo.UNDEFINED);
- boolean rightUnefined = right.equals(RegionInfo.UNDEFINED);
- boolean last = left.isLast();
- boolean first = right.isFirst();
- if (leftUndefined && rightUnefined) {
-LOG.warn("Skipping hole fix; both the hole left-side and right-side
RegionInfos are " +
-"UNDEFINED; left=<{}>, right=<{}>", left, right);
-return holeCover;
- }
- if (leftUndefined || last) {
-holeCover = buildRegionInfo(right.getTable(),
HConstants.EMPTY_START_ROW,
-right.getStartKey());
- } else if (rightUnefined || first) {
-holeCover = buildRegionInfo(left.getTable(), left.getEndKey(),
HConstants.EMPTY_END_ROW);
- } else {
-LOG.warn("Skipping hole fix; don't know what to do with left=<{}>,
right=<{}>",
-left, right);
-return holeCover;
+LOG.warn("Skipping hole fix; left-side endKey is not less than
right-side startKey;"
+ + " left=<{}>, right=<{}>", left, right);
+return Optional.empty();
}
+ return
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r355132861
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestAssignmentManager.java
##
@@ -171,17 +171,11 @@ public void testAssignAnAssignedRegion() throws
Exception {
// wait first assign
waitOnFuture(futureA);
am.getRegionStates().isRegionInState(hri, State.OPEN);
-// Second should be a noop. We should recognize region is already OPEN
internally
-// and skip out doing nothing.
-// wait second assign
-Future futureB = submitProcedure(createAssignProcedure(hri));
-waitOnFuture(futureB);
-am.getRegionStates().isRegionInState(hri, State.OPEN);
-// TODO: What else can we do to ensure just a noop.
+assertNull(createAssignProcedure(hri));
// TODO: Though second assign is noop, it's considered success, can noop
be handled in a
// better way?
-assertEquals(assignSubmittedCount + 2,
assignProcMetrics.getSubmittedCounter().getCount());
+assertEquals(assignSubmittedCount + 1,
assignProcMetrics.getSubmittedCounter().getCount());
Review comment:
Why does the count change?
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r355686750
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
##
@@ -724,24 +731,57 @@ static int compare(TransitRegionStateProcedure left,
TransitRegionStateProcedure
return RegionInfo.COMPARATOR.compare(left.getRegion(), right.getRegion());
}
- private TransitRegionStateProcedure createAssignProcedure(RegionStateNode
regionNode,
- ServerName targetServer, boolean override) {
-TransitRegionStateProcedure proc;
+ private TransitRegionStateProcedure createAssignProcedure(final RegionInfo
regionInfo)
+throws HBaseIOException {
+return createAssignProcedure(regionInfo, null);
+ }
+
+ private TransitRegionStateProcedure createAssignProcedure(final RegionInfo
regionInfo,
+final ServerName targetServer) throws HBaseIOException {
+// TODO: should we use getRegionStateNode?
+final RegionStateNode regionNode =
regionStates.getOrCreateRegionStateNode(regionInfo);
+return createAssignProcedure(regionNode, targetServer, false);
+ }
+
+ /**
+ * Queue a new {@link TransitRegionStateProcedure} representing this
assignment request.
+ * @param regionNode the region to assign
+ * @param targetServer the target server for this assignment (optional)
+ * @param override when {@code true}, remove any existing procedure found on
{@code regionNode}
+ */
+ private TransitRegionStateProcedure createAssignProcedure(final
RegionStateNode regionNode,
+final ServerName targetServer, final boolean override) throws
HBaseIOException {
+final TransitRegionStateProcedure proc;
regionNode.lock();
try {
- if(override && regionNode.getProcedure() != null) {
+ if (override && regionNode.getProcedure() != null) {
regionNode.unsetProcedure(regionNode.getProcedure());
}
assert regionNode.getProcedure() == null;
- proc = TransitRegionStateProcedure.assign(getProcedureEnvironment(),
-regionNode.getRegionInfo(), targetServer);
+ preTransitCheck(regionNode, STATES_EXPECTED_ON_ASSIGN);
Review comment:
So this is new. There are states taht need to be added to
STATES_EXPECTED_ON_ASSIGN given the errors you see?
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r355682676
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +87,176 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
return;
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
-}
+
+LOG.info("Identified {} region holes to fix.", holes.size());
+
+final List newRegionInfos = mapHolesToRegionInfos(holes);
+final List newRegionDirectories =
+ createRegionDirectories(masterServices, newRegionInfos);
+final List newMetaEntries = createMetaEntries(masterServices,
newRegionDirectories);
+final List scheduledProcedures =
+
masterServices.getAssignmentManager().createAssignProceduresBestEffort(newMetaEntries);
+LOG.info(
+ "Scheduled {}/{} new regions for assignment.",
scheduledProcedures.size(), holes.size());
}
/**
- * @return Calculated RegionInfo that covers the hole hole
+ * Create a new {@link RegionInfo} corresponding to each provided "hole"
pair.
*/
- private RegionInfo getHoleCover(Pair hole) {
-RegionInfo holeCover = null;
-RegionInfo left = hole.getFirst();
-RegionInfo right = hole.getSecond();
+ private static List mapHolesToRegionInfos(
Review comment:
Good making it static.
We are creating RIs in here, no? Not 'map'ping holes to RIs? Gives
impression the RIs were there already rather than being created.
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r355132830
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestAssignmentManager.java
##
@@ -171,17 +171,11 @@ public void testAssignAnAssignedRegion() throws
Exception {
// wait first assign
waitOnFuture(futureA);
am.getRegionStates().isRegionInState(hri, State.OPEN);
-// Second should be a noop. We should recognize region is already OPEN
internally
-// and skip out doing nothing.
-// wait second assign
-Future futureB = submitProcedure(createAssignProcedure(hri));
-waitOnFuture(futureB);
-am.getRegionStates().isRegionInState(hri, State.OPEN);
-// TODO: What else can we do to ensure just a noop.
+assertNull(createAssignProcedure(hri));
Review comment:
Good
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r355132615
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +87,176 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
Review comment:
I like the improved log message but not change in log level. Ideally CJ
would output one summary line rather than one per feature.
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #917: HBASE-23383 [hbck2] `fixHoles` should queue assignment procedures for any regions its fixing
saintstack commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r355680821
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaFixer.java
##
@@ -86,74 +87,176 @@ void fix() throws IOException {
* If hole, it papers it over by adding a region in the filesystem and to
hbase:meta.
* Does not assign.
*/
- void fixHoles(CatalogJanitor.Report report) throws IOException {
-List> holes = report.getHoles();
+ void fixHoles(CatalogJanitor.Report report) {
+final List> holes = report.getHoles();
if (holes.isEmpty()) {
- LOG.debug("No holes.");
+ LOG.info("CatalogJanitor Report contains no holes to fix. Skipping.");
return;
}
-for (Pair p: holes) {
- RegionInfo ri = getHoleCover(p);
- if (ri == null) {
-continue;
- }
- Configuration configuration = this.masterServices.getConfiguration();
- HRegion.createRegionDir(configuration, ri,
FSUtils.getRootDir(configuration));
- // If an error here, then we'll have a region in the filesystem but not
- // in hbase:meta (if the below fails). Should be able to rerun the fix.
- // Add to hbase:meta and then update in-memory state so it knows of new
- // Region; addRegionToMeta adds region and adds a state column set to
CLOSED.
- MetaTableAccessor.addRegionToMeta(this.masterServices.getConnection(),
ri);
- this.masterServices.getAssignmentManager().getRegionStates().
- updateRegionState(ri, RegionState.State.CLOSED);
- LOG.info("Fixed hole by adding {} in CLOSED state; region NOT assigned
(assign to ONLINE).",
- ri);
-}
+
+LOG.info("Identified {} region holes to fix.", holes.size());
Review comment:
Need this at INFO when we have another log line below at INFO on what we are
going to 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
With regards,
Apache Git Services
