Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 0.95.2
    • Fix Version/s: None
    • Component/s: Client, master, regionserver
    • Labels:
    • Tags:
      noob

      Description

      This is bad...

          public boolean accept(Path p) {
            boolean isValid = false;
            try {
              if (HConstants.HBASE_NON_USER_TABLE_DIRS.contains(p.toString())) {
                isValid = false;
              } else {
                  isValid = this.fs.getFileStatus(p).isDir();
              }
            } catch (IOException e) {
              e.printStackTrace();          <================ 
            }
            return isValid;
          }
        }
      
      1. HBASE-6356.patch
        0.7 kB
        Gustavo Anatoly

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #259 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/259/)
          HBASE-6356 printStackTrace in FSUtils (Gustavo Anatoly) (Revision 1408851)

          Result = FAILURE
          nkeywal :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #259 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/259/ ) HBASE-6356 printStackTrace in FSUtils (Gustavo Anatoly) (Revision 1408851) Result = FAILURE nkeywal : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #3536 (See https://builds.apache.org/job/HBase-TRUNK/3536/)
          HBASE-6356 printStackTrace in FSUtils (Gustavo Anatoly) (Revision 1408851)

          Result = FAILURE
          nkeywal :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #3536 (See https://builds.apache.org/job/HBase-TRUNK/3536/ ) HBASE-6356 printStackTrace in FSUtils (Gustavo Anatoly) (Revision 1408851) Result = FAILURE nkeywal : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
          Hide
          Gustavo Anatoly added a comment -

          Thanks you Nicolas, for your patience and your reviews.

          Show
          Gustavo Anatoly added a comment - Thanks you Nicolas, for your patience and your reviews.
          Hide
          Nicolas Liochon added a comment -

          Committed and jira assigned to you, Gustavo. Thanks for the patch!.

          Show
          Nicolas Liochon added a comment - Committed and jira assigned to you, Gustavo. Thanks for the patch!.
          Hide
          stack added a comment -

          +1 on commit

          Show
          stack added a comment - +1 on commit
          Hide
          Gustavo Anatoly added a comment -

          Hi, Nicolas.

          Excuse me for the typo. I corrected in this new patch.

          Thanks.

          Show
          Gustavo Anatoly added a comment - Hi, Nicolas. Excuse me for the typo. I corrected in this new patch. Thanks.
          Hide
          Nicolas Liochon added a comment -

          Seems ok to me, except a typo I can fix on commit. Will do that tomorrow if no one objects.

          Show
          Nicolas Liochon added a comment - Seems ok to me, except a typo I can fix on commit. Will do that tomorrow if no one objects.
          Hide
          Gustavo Anatoly added a comment -

          Hi, Nicolas.

          Thanks for reply. I've attached the patch for review.

          Show
          Gustavo Anatoly added a comment - Hi, Nicolas. Thanks for reply. I've attached the patch for review.
          Hide
          Nicolas Liochon added a comment -

          I was wondering what Path#toString() exactly returns. There is no javadoc (at least in version 1.1), but I checked the source code, and yes, it returns the full patch, so I think it's ok to use it as you did.

          Show
          Nicolas Liochon added a comment - I was wondering what Path#toString() exactly returns. There is no javadoc (at least in version 1.1), but I checked the source code, and yes, it returns the full patch, so I think it's ok to use it as you did.
          Hide
          Gustavo Anatoly added a comment -

          Hi, Nicolas.

          About the message LOG.warn I think is good, because the log reader will understand what happened, and continue with a not valid return, but about your second question, would you like that check if Path#toString() is really a name before passed to the LOG.warn?

          Thanks for your patience.

          Show
          Gustavo Anatoly added a comment - Hi, Nicolas. About the message LOG.warn I think is good, because the log reader will understand what happened, and continue with a not valid return, but about your second question, would you like that check if Path#toString() is really a name before passed to the LOG.warn? Thanks for your patience.
          Hide
          Nicolas Liochon added a comment -

          Hi Gustavo,

          What do you think of
          LOG.warn("An error occured while verifying [" + p.toString() + "] if is a valid directory. Returning 'not valid' and continuing.", e);

          It's worth checking what Path#toString() says as well (is it really a name?)

          Thanks!

          Nicolas

          Show
          Nicolas Liochon added a comment - Hi Gustavo, What do you think of LOG.warn("An error occured while verifying [" + p.toString() + "] if is a valid directory. Returning 'not valid' and continuing.", e); It's worth checking what Path#toString() says as well (is it really a name?) Thanks! Nicolas
          Hide
          Gustavo Anatoly added a comment -

          Hi, nkeywal.

          Could you verify if you are agree with this message to LOG.warn?

          public boolean accept(Path p) {
            boolean isValid = false;
            try {
              if (HConstants.HBASE_NON_USER_TABLE_DIRS.contains(p.toString())) {
                isValid = false;
              } else {
                isValid = this.fs.getFileStatus(p).isDir();
              }
            } catch (IOException e) {
              LOG.warn("An error occured while verify [" + p.toString() + "] if is a valid directory.", e);
            }
            return isValid;
          }
          

          If you are agree, I will submit a new patch.

          Thanks.

          Show
          Gustavo Anatoly added a comment - Hi, nkeywal. Could you verify if you are agree with this message to LOG.warn? public boolean accept(Path p) { boolean isValid = false; try { if (HConstants.HBASE_NON_USER_TABLE_DIRS.contains(p.toString())) { isValid = false; } else { isValid = this.fs.getFileStatus(p).isDir(); } } catch (IOException e) { LOG.warn("An error occured while verify [" + p.toString() + "] if is a valid directory.", e); } return isValid; } If you are agree, I will submit a new patch. Thanks.
          Hide
          Gustavo Anatoly added a comment -

          Hi, nkeywal.

          Really I'm wrong, I will fix it.

          Thanks for your advice

          Show
          Gustavo Anatoly added a comment - Hi, nkeywal. Really I'm wrong, I will fix it. Thanks for your advice
          Hide
          Nicolas Liochon added a comment -

          Hum, if I'm not wrong the exception will come only from this.fs.getFileStatus(p).isDir();?
          So it's not in the HConstants.HBASE_NON_USER_TABLE_DIRS list?

          Show
          Nicolas Liochon added a comment - Hum, if I'm not wrong the exception will come only from this.fs.getFileStatus(p).isDir();? So it's not in the HConstants.HBASE_NON_USER_TABLE_DIRS list?
          Hide
          Gustavo Anatoly added a comment -

          Hi, nkeywal.

          A patch has been uploaded to review, could verify if were that you had thought?

          Thanks.

          Show
          Gustavo Anatoly added a comment - Hi, nkeywal. A patch has been uploaded to review, could verify if were that you had thought? Thanks.
          Hide
          Gustavo Anatoly added a comment -

          printStackTrace refactored

          Show
          Gustavo Anatoly added a comment - printStackTrace refactored
          Hide
          Gustavo Anatoly added a comment -

          Hi, nkeywal

          Thanks for reply firstly. Yes, I would like to work in this task, I will work on a patch to submit for review, adding a message on log.

          Thanks again.

          Show
          Gustavo Anatoly added a comment - Hi, nkeywal Thanks for reply firstly. Yes, I would like to work in this task, I will work on a patch to submit for review, adding a message on log. Thanks again.
          Hide
          Nicolas Liochon added a comment -

          Hi Gustavo,

          Yes, it would work. It's the simple option. But it's clean. I would add a message in the log, especially mentioning that we're ignoring the error and returning false.

          Do you want to submit a patch?

          Thanks,

          Show
          Nicolas Liochon added a comment - Hi Gustavo, Yes, it would work. It's the simple option. But it's clean. I would add a message in the log, especially mentioning that we're ignoring the error and returning false. Do you want to submit a patch? Thanks,
          Hide
          Gustavo Anatoly added a comment -

          Hi, nkeywal.

          I read about your comment and I make a simple update like this:

          public boolean accept(Path p) {
                boolean isValid = false;
                try {
                  if (HConstants.HBASE_NON_USER_TABLE_DIRS.contains(p.toString())) {
                    isValid = false;
                  } else {
                      isValid = this.fs.getFileStatus(p).isDir();
                  }
                } catch (IOException e) {
                  LOG.warn(e);
                }
                return isValid;
          }
          

          Could you verify if that code above is like you was thinking?

          Thanks.

          Show
          Gustavo Anatoly added a comment - Hi, nkeywal. I read about your comment and I make a simple update like this: public boolean accept(Path p) { boolean isValid = false; try { if (HConstants.HBASE_NON_USER_TABLE_DIRS.contains(p.toString())) { isValid = false; } else { isValid = this.fs.getFileStatus(p).isDir(); } } catch (IOException e) { LOG.warn(e); } return isValid; } Could you verify if that code above is like you was thinking? Thanks.
          Hide
          Nicolas Liochon added a comment -

          Yes, we're not allowed to throw. I wonder if it's better to log.warn or to stop here. Today when it happens it returns false, so the easy option is to just log and saying that we keep the backward compatibility this way...

          Show
          Nicolas Liochon added a comment - Yes, we're not allowed to throw. I wonder if it's better to log.warn or to stop here. Today when it happens it returns false, so the easy option is to just log and saying that we keep the backward compatibility this way...
          Hide
          stack added a comment -

          I suppose we're not allowed throw in here? Should we throw a RuntimeException?

          Show
          stack added a comment - I suppose we're not allowed throw in here? Should we throw a RuntimeException?

            People

            • Assignee:
              Gustavo Anatoly
              Reporter:
              Nicolas Liochon
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development