Fix style errors

Author: arunkumarchacko

hadoop-tools/hadoop-gcp/dev-support/findbugs-exclude.xml

@@ -0,0 +1,89 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<FindBugsFilter>
+
+  <!-- same code as in FileSystem is triggering the same warning. -->
+  <Match>
+    <Class name="org.apache.hadoop.fs.s3native.S3xLoginHelper" />
+    <Method name="checkPath" />
+    <Bug pattern="ES_COMPARING_STRINGS_WITH_EQ" />
+  </Match>
+  <!-- Redundant null check makes code clearer, future-proof here. -->
+  <Match>
+    <Class name="org.apache.hadoop.fs.s3a.S3AFileSystem" />
+    <Method name="s3Exists" />
+    <Bug pattern="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE" />
+  </Match>
+  <!-- we are using completable futures, so ignore the Future which submit() returns -->
+  <Match>
+    <Class name="org.apache.hadoop.fs.s3a.impl.InputStreamCallbacksImpl" />
+    <Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE" />
+  </Match>
+
+  <!--
+   findbugs gets confused by lambda expressions in synchronized methods
+   and considers references to fields to be unsynchronized.
+   As you can't disable the methods individually, we have to disable
+   them for the entire class.
+    -->
+  <Match>
+    <Class name="org.apache.hadoop.fs.s3a.S3AInputStream"/>
+    <Bug pattern="IS2_INCONSISTENT_SYNC"/>
+  </Match>
+  <!--
+    findbugs reporting RV ignored. Not true.
+    "Return value of S3AReadOpContext.getReadInvoker() ignored,
+    but method has no side effect"
+  -->
+  <Match>
+    <Class name="org.apache.hadoop.fs.s3a.S3AInputStream"/>
+    <Method name="reopen"/>
+    <Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT"/>
+  </Match>
+  <Match>
+    <Class name="org.apache.hadoop.fs.s3a.S3AFileSystem"/>
+    <Method name="openFileWithOptions"/>
+    <Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE"/>
+  </Match>
+  <Match>
+    <Class name="org.apache.hadoop.fs.s3a.S3AFileSystem"/>
+    <Field name="futurePool"/>
+    <Bug pattern="IS2_INCONSISTENT_SYNC"/>
+  </Match>
+  <Match>
+    <Class name="org.apache.hadoop.fs.s3a.s3guard.S3GuardTool$BucketInfo"/>
+    <Method name="run"/>
+    <Bug pattern="SF_SWITCH_FALLTHROUGH"/>
+  </Match>
+
+  <!--
+  Some of the S3A Instrumentation classes increment volatile references from
+  within synchronized contexts; they use volatile to keep the cost
+  of these updates and reading them down.
+   -->
+  <Match>
+    <Class name="org.apache.hadoop.fs.s3a.S3AInstrumentation$InputStreamStatisticsImpl"/>
+    <Bug pattern="VO_VOLATILE_INCREMENT"/>
+  </Match>
+
+  <!-- Ignore return value from this method call -->
+  <Match>
+    <Class name="org.apache.hadoop.fs.s3a.impl.StoreContext"/>
+    <Method name="submit"/>
+    <Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE"/>
+  </Match>
+</FindBugsFilter>

hadoop-tools/hadoop-gcp/src/main/java/org/apache/hadoop/fs/gs/Constants.java

@@ -18,7 +18,9 @@
 
 package org.apache.hadoop.fs.gs;
 
-class Constants {
+final class Constants {
+  private Constants() {}
+
   // URI scheme for GCS.
   static final String SCHEME = "gs";
   static final String PATH_DELIMITER = "/";

hadoop-tools/hadoop-gcp/src/main/java/org/apache/hadoop/fs/gs/CreateOptions.java

@@ -29,17 +29,15 @@
 /**
  * Options that can be specified when creating a file in the {@link GoogleCloudStorageFileSystem}.
  */
-class CreateOptions {
+final class CreateOptions {
   private final ImmutableMap<String, byte[]> attributes;
   private final String contentType;
   private final boolean ensureNoDirectoryConflict;
   private final Duration interval;
   private final long overwriteGenerationId;
   private final WriteMode mode;
 
-  public static final CreateOptions DEFAULT = builder().build();
-
-  public String getContentEncoding() {
+  String getContentEncoding() {
     return contentEncoding;
   }
 
@@ -51,19 +49,15 @@ private CreateOptions(CreateOperationOptionsBuilder builder) {
     this.ensureNoDirectoryConflict = builder.ensureNoDirectoryConflict;
     this.interval = builder.interval;
     this.overwriteGenerationId = builder.overwriteGenerationId;
-    this.mode = builder.mode;
+    this.mode = builder.writeMode;
     this.contentEncoding = builder.contentEncoding;
   }
 
-  public boolean isOverwriteExisting() {
+  boolean isOverwriteExisting() {
     return this.mode == WriteMode.OVERWRITE;
   }
 
   enum WriteMode {
-    /**
-     * Write new bytes to the end of the existing file rather than the beginning.
-     */
-    APPEND,
     /**
      * Creates a new file for write and fails if file already exists.
      */
@@ -74,45 +68,29 @@ enum WriteMode {
     OVERWRITE
   }
 
-  public static CreateOperationOptionsBuilder builder() {
+  static CreateOperationOptionsBuilder builder() {
     return new CreateOperationOptionsBuilder();
   }
 
   /**
    * Extended attributes to set when creating a file.
    */
-  public ImmutableMap<String, byte[]> getAttributes() {
+  ImmutableMap<String, byte[]> getAttributes() {
     return attributes;
   }
 
   /**
    * Content-type to set when creating a file.
    */
   @Nullable
-  public String getContentType() {
+  String getContentType() {
     return contentType;
   }
 
-  /**
-   * Configures the minimum time interval (milliseconds) between consecutive sync/flush calls
-   */
-  public Duration getMinSyncInterval() {
-    return interval;
-  }
-
-  /**
-   * If true, makes sure there isn't already a directory object of the same name. If false, you run
-   * the risk of creating hard-to-cleanup/access files whose names collide with directory names. If
-   * already sure no such directory exists, then this is safe to set for improved performance.
-   */
-  public boolean isEnsureNoDirectoryConflict() {
-    return ensureNoDirectoryConflict;
-  }
-
   /**
    * Whether to overwrite an existing file with the same name.
    */
-  public WriteMode getWriteMode() {
+  WriteMode getWriteMode() {
     return mode;
   }
 
@@ -123,7 +101,7 @@ public WriteMode getWriteMode() {
    * only be overwritten by the newly created file if its generation matches this provided
    * generationId.
    */
-  public long getOverwriteGenerationId() {
+  long getOverwriteGenerationId() {
     return overwriteGenerationId;
   }
 
@@ -133,38 +111,12 @@ static class CreateOperationOptionsBuilder {
     private boolean ensureNoDirectoryConflict = true;
     private Duration interval = Duration.ZERO;
     private long overwriteGenerationId = StorageResourceId.UNKNOWN_GENERATION_ID;
-    private WriteMode mode = WriteMode.CREATE_NEW;
+    private WriteMode writeMode = WriteMode.CREATE_NEW;
 
     private String contentEncoding = null;
 
-    public CreateOperationOptionsBuilder setAttributes(Map<String, byte[]> attributes) {
-      this.attributes = attributes;
-      return this;
-    }
-
-    public CreateOperationOptionsBuilder setContentType(String contentType) {
-      this.contentType = contentType;
-      return this;
-    }
-
-    public CreateOperationOptionsBuilder setEnsureNoDirectoryConflict(
-        boolean ensureNoDirectoryConflict) {
-      this.ensureNoDirectoryConflict = ensureNoDirectoryConflict;
-      return this;
-    }
-
-    public CreateOperationOptionsBuilder setMinSyncInterval(Duration interval) {
-      this.interval = interval;
-      return this;
-    }
-
-    public CreateOperationOptionsBuilder setOverwriteGenerationId(long overwriteGenerationId) {
-      this.overwriteGenerationId = overwriteGenerationId;
-      return this;
-    }
-
-    public CreateOperationOptionsBuilder setWriteMode(WriteMode mode) {
-      this.mode = mode;
+    CreateOperationOptionsBuilder setWriteMode(WriteMode mode) {
+      this.writeMode = mode;
       return this;
     }
 

hadoop-tools/hadoop-gcp/src/main/java/org/apache/hadoop/fs/gs/ErrorTypeExtractor.java

@@ -17,23 +17,22 @@
 package org.apache.hadoop.fs.gs;
 
 import io.grpc.Status;
-import io.grpc.StatusRuntimeException;
-
-import javax.annotation.Nullable;
 
 /**
  * Implementation for {@link ErrorTypeExtractor} for exception specifically thrown from gRPC path.
  */
-class ErrorTypeExtractor {
+final class ErrorTypeExtractor {
 
   enum ErrorType {
-    NOT_FOUND, OUT_OF_RANGE, ALREADY_EXISTS, FAILED_PRECONDITION, INTERNAL, RESOURCE_EXHAUSTED, UNAVAILABLE, UNKNOWN
+    NOT_FOUND, OUT_OF_RANGE, ALREADY_EXISTS, FAILED_PRECONDITION, INTERNAL, RESOURCE_EXHAUSTED,
+    UNAVAILABLE, UNKNOWN
   }
 
   //  public static final ErrorTypeExtractor INSTANCE = new ErrorTypeExtractor();
 
   private static final String BUCKET_ALREADY_EXISTS_MESSAGE =
-      "FAILED_PRECONDITION: Your previous request to create the named bucket succeeded and you already own it.";
+      "FAILED_PRECONDITION: Your previous request to create the named bucket succeeded and you "
+          + "already own it.";
 
   private ErrorTypeExtractor() {
   }
@@ -58,39 +57,4 @@ static ErrorType getErrorType(Exception error) {
       return ErrorType.UNKNOWN;
     }
   }
-
-  static boolean bucketAlreadyExists(Exception e) {
-    ErrorType errorType = getErrorType(e);
-    if (errorType == ErrorType.ALREADY_EXISTS) {
-      return true;
-    }
-    // The gRPC API currently throws a FAILED_PRECONDITION status code instead of ALREADY_EXISTS,
-    // so we handle both these conditions in the interim.
-    // TODO: remove once the status codes are fixed.
-    else if (errorType == ErrorType.FAILED_PRECONDITION) {
-      StatusRuntimeException statusRuntimeException = getStatusRuntimeException(e);
-      return statusRuntimeException != null && BUCKET_ALREADY_EXISTS_MESSAGE.equals(
-          statusRuntimeException.getMessage());
-    }
-    return false;
-  }
-
-  /**
-   * Extracts StatusRuntimeException from the Exception, if it exists.
-   */
-  @Nullable
-  static private StatusRuntimeException getStatusRuntimeException(Exception e) {
-    Throwable cause = e;
-    // Keeping a counter to break early from the loop to avoid infinite loop condition due to
-    // cyclic exception chains.
-    int currentExceptionDepth = 0, maxChainDepth = 1000;
-    while (cause != null && currentExceptionDepth < maxChainDepth) {
-      if (cause instanceof StatusRuntimeException) {
-        return (StatusRuntimeException) cause;
-      }
-      cause = cause.getCause();
-      currentExceptionDepth++;
-    }
-    return null;
-  }
 }