[GitHub] [hbase-operator-tools] taklwu commented on a change in pull request #88: HBASE-25921 Fix Wrong FileSystem when running `filesystem` on non-HDFS storage

[GitBox <gi...@apache.org>]

taklwu commented on a change in pull request #88:
URL: https://github.com/apache/hbase-operator-tools/pull/88#discussion_r640115793



##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/hbck1/HBaseFsck.java
##########
@@ -1535,7 +1535,7 @@ private void loadHdfsRegioninfo(HbckInfo hbi) throws IOException {
       return;
     }
 
-    FileSystem fs = FileSystem.get(getConf());
+    FileSystem fs = FSUtils.getCurrentFileSystem(getConf());

Review comment:
       good to know, I have updated to use `CommonFSUtils` and further `HBCKFSUtils`. 
   
   for whatever I touch here, I can of coz use/implement `HBCKFSUtils`, but again.....I felt like we should not re-implement the same thing across `hbase-common` and `hbase-operator-tools` (sorry I'm a bit confused) So, for this change, should we use Utilities class of `CommonFSUtils` and further make it `InterfaceAudience.Public` (sorry that I just don't like copying code)? 
   
   I'm learning, IMO the standard of using `InterfaceAudience.Private` outside of hbase core is a bit higher that if we think we should not use any `InterfaceAudience.Private` classes in `hbase-common` or `hbase-common` itself should not be used in `hbase-operator-tools`, we may need to have a larger refactoring to copy/implement few more functions from `CommonFSUtils` and `FSUtils` to `HBCKFSUtils`. however, to limit the change/scope of this bug fix and I found more classes in this package make use of `CommonFSUtils` and `FSUtils`, I would propose and suggest to have a follow-up PR if 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:
users@infra.apache.org