From 123960ad31f9258d2dadd788029941bb984ae0fa Mon Sep 17 00:00:00 2001 From: losalex <90795544+losalex@users.noreply.github.com> Date: Wed, 2 Nov 2022 23:54:44 +0200 Subject: [PATCH] fix: Make partialSuccess to be true by default (#1173) * fix: Make partialSuccess to be true by default * Address PR comments * Move back Instrumentation.addPartialSuccessOption --- .../com/google/cloud/logging/LoggingImpl.java | 13 +++++++--- .../google/cloud/logging/LoggingImplTest.java | 24 +++++++++++++++++++ 2 files changed, 34 insertions(+), 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 index 8c813feb1..48a5f0f85 100644 --- 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 logEntries, WriteOption... options) { try { final Map 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> pair = @@ -872,9 +874,14 @@ public void write(Iterable 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 index 9f230d60b..7504299ed 100644 --- 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 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))