Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Consumer;
import java.util.function.Supplier;
Expand Down Expand Up @@ -184,6 +185,7 @@ public boolean shouldRetry(IOException e) {
return RetryDeterminer.SOCKET_ERRORS.shouldRetry(e);
}
};
private static final AtomicBoolean overwriteLog = new AtomicBoolean(false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The variable name overwriteLog is somewhat ambiguous and could be interpreted as overwriting a log file. Renaming it to something like gcsioLoggerConfigured would more clearly indicate that it tracks whether the logging configuration for the GCS IO library has been applied.

Suggested change
private static final AtomicBoolean overwriteLog = new AtomicBoolean(false);
private static final AtomicBoolean gcsioLoggerConfigured = new AtomicBoolean(false);


/////////////////////////////////////////////////////////////////////////////

Expand Down Expand Up @@ -726,9 +728,16 @@ public WritableByteChannel create(GcsPath path, CreateOptions options) throws IO
}
}

@SuppressFBWarnings("LG_LOST_LOGGER_DUE_TO_WEAK_REFERENCE")
GoogleCloudStorage createGoogleCloudStorage(
GoogleCloudStorageOptions options, Storage storage, Credentials credentials)
throws IOException {
// Suppress log spams in gcsio 3.0
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested working with ./gradlew :sdks:java:testing:jpms-tests:dataflowRunnerIntegrationTest -info. If put it in a static block it's not effective because the root logger hasn't been initialized yet

if (overwriteLog.compareAndSet(false, true)) {
java.util.logging.Logger.getLogger("com.google.cloud.hadoop.gcsio.GoogleCloudStorageImpl")
.setLevel(java.util.logging.Level.SEVERE);
}
Comment on lines +736 to +739
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using GoogleCloudStorageImpl.class.getName() is preferred over a hardcoded string to ensure type safety and ease of refactoring. Additionally, please note that setting the log level to SEVERE is very aggressive as it suppresses all WARNING and INFO messages from this class, which might contain important diagnostic information. If the intention is only to suppress the specific "ALERT" message, consider if WARNING is sufficient or if this suppression should be made configurable.

    if (gcsioLoggerConfigured.compareAndSet(false, true)) {
      java.util.logging.Logger gcsLogger =
          java.util.logging.Logger.getLogger(GoogleCloudStorageImpl.class.getName());
      gcsLogger.setLevel(java.util.logging.Level.SEVERE);
    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spamming log is a WARN log, see PR description


return GoogleCloudStorageImpl.builder()
.setOptions(options)
.setHttpTransport(storage.getRequestFactory().getTransport())
Expand Down
Loading