Copilot commented on code in PR #154:
URL:
https://github.com/apache/hbase-operator-tools/pull/154#discussion_r2432938131
##########
hbase-hbck2/src/main/java/org/apache/hbase/hbck1/HBaseFsck.java:
##########
@@ -582,7 +586,7 @@ public void connect() throws IOException {
Runtime.getRuntime().addShutdownHook(new Thread() {
@Override
public void run() {
- IOUtils.closeQuietly(HBaseFsck.this);
+ close();
cleanupHbckZnode();
unlockHbck();
Review Comment:
close() already calls cleanupHbckZnode() and unlockHbck(); invoking them
again in the shutdown hook duplicates work and can cause redundant
deletes/logging. Remove the extra cleanupHbckZnode() and unlockHbck() calls and
rely on close() to perform the cleanup.
```suggestion
```
##########
hbase-hbck2/src/main/java/org/apache/hbase/hbck1/HBaseFsck.java:
##########
@@ -1876,7 +1886,30 @@ private HRegion createNewMeta() throws IOException {
Path rootdir = CommonFSUtils.getRootDir(getConf());
RegionInfo ri = RegionInfoBuilder.FIRST_META_REGIONINFO;
TableDescriptor td = new
FSTableDescriptors(getConf()).get(TableName.META_TABLE_NAME);
- return HBaseTestingUtility.createRegionAndWAL(ri, rootdir, getConf(), td);
+ ChunkCreator.initialize(MemStoreLAB.CHUNK_SIZE_DEFAULT, false, 0, 0, 0,
null,
+ MemStoreLAB.INDEX_CHUNK_SIZE_PERCENTAGE_DEFAULT);
+ WAL wal = createWal(getConf(), rootdir, ri);
+ return HRegion.createHRegion(ri, rootdir, getConf(), td, wal, true);
+ }
+
+ private static WAL createWal(final Configuration conf, final Path rootDir,
final RegionInfo hri)
+ throws IOException {
+ // The WAL subsystem will use the default rootDir rather than the passed
in rootDir
+ // unless I pass along via the conf.
+ Configuration confForWAL = new Configuration(conf);
+ confForWAL.set(HConstants.HBASE_DIR, rootDir.toString());
+ return new WALFactory(confForWAL, "hregion-" +
RandomStringUtils.randomNumeric(8)).getWAL(hri);
+ }
+
Review Comment:
WALFactory is instantiated but never shut down, which can leak background
threads/resources; closing the WAL does not automatically shut down the
factory. Create the WALFactory in createNewMeta(), keep a reference, and call
factory.shutdown() in the finally block after closeRegionAndWAL(meta). For
example, construct WALFactory factory = new WALFactory(...); WAL wal =
factory.getWAL(ri); and then in the finally after closeRegionAndWAL(meta),
invoke factory.shutdown().
```suggestion
WALFactory factory = null;
WAL wal = null;
HRegion region = null;
try {
Configuration confForWAL = new Configuration(getConf());
confForWAL.set(HConstants.HBASE_DIR, rootdir.toString());
factory = new WALFactory(confForWAL, "hregion-" +
RandomStringUtils.randomNumeric(8));
wal = factory.getWAL(ri);
region = HRegion.createHRegion(ri, rootdir, getConf(), td, wal, true);
return region;
} catch (IOException e) {
// If region or wal creation failed, ensure resources are cleaned up
if (region != null) {
try {
closeRegionAndWAL(region);
} catch (IOException ioe) {
// log or ignore
}
} else if (wal != null) {
try {
wal.close();
} catch (IOException ioe) {
// log or ignore
}
}
throw e;
} finally {
if (factory != null) {
factory.shutdown();
}
}
}
```
##########
hbase-hbck2/src/main/java/org/apache/hbase/hbck1/HBaseFsck.java:
##########
@@ -882,9 +886,14 @@ public void close() throws IOException {
zkw.close();
zkw = null;
}
- IOUtils.closeQuietly(admin);
- IOUtils.closeQuietly(meta);
- IOUtils.closeQuietly(connection);
+ try {
+ Closeables.close(admin, true);
+ Closeables.close(meta, true);
+ Closeables.close(connection, true);
+ } catch (IOException e) {
+ // should not happen
+ throw new AssertionError(e);
+ }
Review Comment:
Closeables.close(..., true) is documented to swallow IOExceptions, so this
catch block is unreachable; converting to AssertionError is also surprising for
a close path. Remove the try/catch and call Closeables.close(..., true)
directly.
```suggestion
Closeables.close(admin, true);
Closeables.close(meta, true);
Closeables.close(connection, true);
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
