fix: Make partialSuccess to be true by default (#1173)

* fix: Make partialSuccess to be true by default * Address PR comments * Move back Instrumentation.addPartialSuccessOption
googleapis · Nov 2, 2022 · 123960a · 123960a
1 parent e836fc3
commit 123960a
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 3 deletions.
diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
@@ -859,6 +859,8 @@ public void write(Iterable<LogEntry> logEntries, WriteOption... options) {
     try {
       final Map<Option.OptionType, ?> writeOptions = optionMap(options);
       final Boolean loggingOptionsPopulateFlag = getOptions().getAutoPopulateMetadata();
+      final Boolean writeOptionPartialSuccessFlag =
+          WriteOption.OptionType.PARTIAL_SUCCESS.get(writeOptions);
       final Boolean writeOptionPopulateFlag =
           WriteOption.OptionType.AUTO_POPULATE_METADATA.get(writeOptions);
       Tuple<Boolean, Iterable<LogEntry>> pair =
@@ -872,9 +874,14 @@ public void write(Iterable<LogEntry> logEntries, WriteOption... options) {
         logEntries =
             populateMetadata(logEntries, sharedResourceMetadata, this.getClass().getName());
       }
-      // Add partialSuccess option always for request containing instrumentation data
-      writeLogEntries(
-          logEntries, pair.x() ? Instrumentation.addPartialSuccessOption(options) : options);
+      // Add partialSuccess = true option always for request which does not have
+      // it set explicitly in options.
+      // For request containing instrumentation data (e.g. when pair.x() == true),
+      // always set or override partialSuccess with true.
+      if (pair.x() || writeOptionPartialSuccessFlag == null) {
+        options = Instrumentation.addPartialSuccessOption(options);
+      }
+      writeLogEntries(logEntries, options);
       if (flushSeverity != null) {
         for (LogEntry logEntry : logEntries) {
           // flush pending writes if log severity at or above flush severity

diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
@@ -1851,6 +1851,7 @@ public void testWriteLogEntries() {
             .addAllEntries(
                 Iterables.transform(
                     ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), LogEntry.toPbFunction(PROJECT)))
+            .setPartialSuccess(true)
             .build();
     WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance();
     EasyMock.expect(loggingRpcMock.write(request)).andReturn(ApiFutures.immediateFuture(response));
@@ -1869,6 +1870,7 @@ public void testWriteLogEntriesDoesNotEnableFlushByDefault() {
                     ImmutableList.of(
                         LOG_ENTRY1, LOG_ENTRY2.toBuilder().setSeverity(Severity.EMERGENCY).build()),
                     LogEntry.toPbFunction(PROJECT)))
+            .setPartialSuccess(true)
             .build();
     ApiFuture<WriteLogEntriesResponse> apiFuture = SettableApiFuture.create();
     EasyMock.expect(loggingRpcMock.write(request)).andReturn(apiFuture);
@@ -1886,6 +1888,7 @@ public void testWriteLogEntriesWithSeverityFlushEnabled() {
             .addAllEntries(
                 Iterables.transform(
                     ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), LogEntry.toPbFunction(PROJECT)))
+            .setPartialSuccess(true)
             .build();
     WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance();
     EasyMock.expect(loggingRpcMock.write(request)).andReturn(ApiFutures.immediateFuture(response));
@@ -1934,6 +1937,7 @@ public void testWriteLogEntriesAsync() {
                     ImmutableList.of(
                         LOG_ENTRY1, LOG_ENTRY2, LOG_ENTRY_NO_DESTINATION, LOG_ENTRY_EMPTY),
                     LogEntry.toPbFunction(PROJECT)))
+            .setPartialSuccess(true)
             .build();
     WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance();
     EasyMock.expect(loggingRpcMock.write(request)).andReturn(ApiFutures.immediateFuture(response));
@@ -1955,6 +1959,7 @@ public void testWriteLogEntriesAsyncWithOptions() {
             .addAllEntries(
                 Iterables.transform(
                     ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), LogEntry.toPbFunction(PROJECT)))
+            .setPartialSuccess(true)
             .build();
     WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance();
     EasyMock.expect(loggingRpcMock.write(request)).andReturn(ApiFutures.immediateFuture(response));
@@ -2229,6 +2234,7 @@ public void testFlush() throws InterruptedException {
             .addAllEntries(
                 Iterables.transform(
                     ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), LogEntry.toPbFunction(PROJECT)))
+            .setPartialSuccess(true)
             .build();
     EasyMock.expect(loggingRpcMock.write(request)).andReturn(mockRpcResponse);
     EasyMock.replay(loggingRpcMock);
@@ -2264,6 +2270,7 @@ public void testFlushStress() throws InterruptedException {
         WriteLogEntriesRequest.newBuilder()
             .addAllEntries(
                 Iterables.transform(ImmutableList.of(LOG_ENTRY1), LogEntry.toPbFunction(PROJECT)))
+            .setPartialSuccess(true)
             .build();
 
     Thread[] threads = new Thread[100];
@@ -2304,6 +2311,22 @@ public void testDiagnosticInfoWithPartialSuccess() {
     testDiagnosticInfoGeneration(true);
   }
 
+  @Test
+  public void testPartialSuccessNotOverridenIfPresent() {
+    WriteLogEntriesRequest request =
+        WriteLogEntriesRequest.newBuilder()
+            .addAllEntries(
+                Iterables.transform(
+                    ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), LogEntry.toPbFunction(PROJECT)))
+            .setPartialSuccess(false)
+            .build();
+    WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance();
+    EasyMock.expect(loggingRpcMock.write(request)).andReturn(ApiFutures.immediateFuture(response));
+    EasyMock.replay(rpcFactoryMock, loggingRpcMock);
+    logging = options.getService();
+    logging.write(ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), WriteOption.partialSuccess(false));
+  }
+
   private void testDiagnosticInfoGeneration(boolean addPartialSuccessOption) {
     Instrumentation.setInstrumentationStatus(false);
     LogEntry jsonEntry =
@@ -2362,6 +2385,7 @@ private void testWriteLogEntriesWithDestination(
                         LOG_ENTRY_NO_DESTINATION,
                         LOG_ENTRY_EMPTY),
                     LogEntry.toPbFunction(projectId)))
+            .setPartialSuccess(true)
             .build();
     WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance();
     EasyMock.expect(loggingRpcMock.write(expectedWriteLogEntriesRequest))