fix(storage): fix junit-vintage-engine dependencies in gapic-google-cloud-storage-v2#13234
fix(storage): fix junit-vintage-engine dependencies in gapic-google-cloud-storage-v2#13234nidhiii-27 wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the QuickstartOpenTelemetrySample.java snippet to include a custom SpanExporter and several Google Cloud Storage operations within a try-finally block. Review feedback highlights that the changes likely contain local debugging code that should not be committed, such as hardcoded project IDs, specific bucket names, and commented-out lines. Additionally, the reviewer pointed out an unused import and a critical issue where exceptions are swallowed, specifically advising on the proper handling of InterruptedException to restore the thread's interrupted status.
| catch (Exception e) { | ||
|
|
||
| } |
There was a problem hiding this comment.
Swallowing exceptions is dangerous. Specifically, for InterruptedException, you must restore the thread's interrupted status by calling Thread.currentThread().interrupt() and handle the interruption appropriately.
catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException("Operation interrupted", e);
} catch (Exception e) {
throw new RuntimeException(e);
}References
- In Java, do not swallow InterruptedException. When catching it, restore the thread's interrupted status by calling Thread.currentThread().interrupt() and handle the interruption appropriately.
| .addSpanProcessor(BatchSpanProcessor.builder(cloudTraceExporter).build()) | ||
| .build()) | ||
| .build(); | ||
| StorageOptions options = StorageOptions.newBuilder().setProjectId("nidhi-consumer").setOpenTelemetry(openTelemetry).build(); |
There was a problem hiding this comment.
Avoid hardcoding project IDs like "nidhi-consumer" in sample code. Use StorageOptions.getDefaultInstance().getProjectId() or allow the library to auto-detect the project from the environment to ensure the sample is portable.
| StorageOptions options = StorageOptions.newBuilder().setProjectId("nidhi-consumer").setOpenTelemetry(openTelemetry).build(); | |
| StorageOptions options = StorageOptions.newBuilder().setOpenTelemetry(openTelemetry).build(); |
| Storage storage = options.getService(); | ||
| try { | ||
| System.out.println("Created an instance of storage with OpenTelemetry configured"); | ||
| Page<Blob> blobs = storage.list("nidhi-test-02"); |
There was a problem hiding this comment.
The bucket name "nidhi-test-02" is hardcoded. For public samples, it is recommended to use placeholders (e.g., "your-bucket-name") or variables so that users can easily adapt the code to their own environment.
| import com.google.api.gax.paging.Page; | ||
| import com.google.cloud.storage.Storage; | ||
| import com.google.cloud.storage.StorageOptions; | ||
| import com.google.cloud.storage.GrpcStorageOptions; |
There was a problem hiding this comment.
The import com.google.cloud.storage.GrpcStorageOptions is unused in this file and should be removed.
| import io.opentelemetry.semconv.ResourceAttributes; | ||
|
|
||
| // [START storage_enable_otel_tracing] | ||
| public class QuickstartOpenTelemetrySample { |
There was a problem hiding this comment.
The extensive changes to this sample file, including hardcoded project IDs and bucket names, appear to be local debugging code that may have been committed accidentally. These changes do not seem to align with the PR title regarding JUnit dependencies.
| // Allow background layout fetch to complete | ||
| Thread.sleep(500); | ||
| Bucket bucket = storage.get("nidhi-test-080808080808080"); | ||
| // storage.list("test"); |
There was a problem hiding this comment.
Remove commented-out code to maintain the quality and readability of the sample.
1 participant
No description provided.