New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Retry S3 task log fetch #14714

Merged

kfaraz merged 10 commits into apache:master from YongGang:retry-s3-task-status

Aug 3, 2023

Contributor

YongGang commented Aug 1, 2023

Description

Saw the following error when fetching task status from S3. This is due to S3 rate limiting on query, we should retry the operation in this case.

com.amazonaws.services.s3.model.AmazonS3Exception: Slow Down (Service: Amazon S3; Status Code: 503; Error Code: 503 Slow Down; Request ID:; S3 Extended Request ID: ; Proxy: null)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.handleErrorResponse(AmazonHttpClient.java:1879) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.handleServiceErrorResponse(AmazonHttpClient.java:1418) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeOneRequest(AmazonHttpClient.java:1387) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeHelper(AmazonHttpClient.java:1157) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.doExecute(AmazonHttpClient.java:814) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeWithTimer(AmazonHttpClient.java:781) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.execute(AmazonHttpClient.java:755) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.access$500(AmazonHttpClient.java:715) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.http.AmazonHttpClient$RequestExecutionBuilderImpl.execute(AmazonHttpClient.java:697) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.http.AmazonHttpClient.execute(AmazonHttpClient.java:561) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.http.AmazonHttpClient.execute(AmazonHttpClient.java:541) ~[aws-java-sdk-core-1.12.317.jar:?]
	at com.amazonaws.services.s3.AmazonS3Client.invoke(AmazonS3Client.java:5456) ~[aws-java-sdk-s3-1.12.317.jar:?]
	at com.amazonaws.services.s3.AmazonS3Client.invoke(AmazonS3Client.java:5403) ~[aws-java-sdk-s3-1.12.317.jar:?]
	at com.amazonaws.services.s3.AmazonS3Client.getObjectMetadata(AmazonS3Client.java:1372) ~[aws-java-sdk-s3-1.12.317.jar:?]
	at org.apache.druid.storage.s3.ServerSideEncryptingAmazonS3.getObjectMetadata(ServerSideEncryptingAmazonS3.java:97) ~[?:?]
	at org.apache.druid.storage.s3.S3TaskLogs.streamTaskFile(S3TaskLogs.java:90) ~[?:?]

Now change to use S3Utils.retryS3Operation for task log fetch, same as task file push method.

Release note

Retry S3 task log fetch

Key changed/added classes in this PR

make streamTaskFile method retry on transient S3 error in S3TaskLogs class

This PR has:


          Retry S3 task log fetch

46dcdbc

github-advanced-security bot found potential problems

View reviewed changes

extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3TaskLogs.java Fixed Show fixed Hide fixed


          add test

a989a12

github-advanced-security bot found potential problems

View reviewed changes

extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3TaskLogsTest.java Fixed Show fixed Hide fixed

YongGang added 2 commits

August 1, 2023 11:26


          fix potential input resource leak warning

a6b1936


          address CodeQL scan issue

7ba24bf

kfaraz reviewed

View reviewed changes

extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3TaskLogs.java Outdated

		@@ -87,38 +92,42 @@ public Optional<InputStream> streamTaskStatus(String taskid) throws IOException
		private Optional<InputStream> streamTaskFile(final long offset, String taskKey) throws IOException

Contributor

kfaraz Aug 2, 2023 •

edited

Loading

It might be nicer to just add a new method streamTaskFileWithRetry, which calls the existing streamTaskFile.

something like:

private Optional<InputStream> streamTaskFileWithRetry(final long offset, String taskKey)
{
  try {
    return S3Utils.retryOperation(() -> streamTaskFile(offset, taskKey))
  }
  catch (Exception e) {
    throw new IOE(e, "Failed to stream logs from: %s", taskKey);
  }
}

The new method can also have a javadoc to mention which failure cases are retried.

Contributor Author

YongGang Aug 2, 2023 •

edited

Loading

Note: the existing streamTaskFile method could throw some wrapped exception e.g. new IOE(e, "Failed to stream logs from: %s", taskKey), in that case the outer new method streamTaskFileWithRetry won't know whether should retry or not. That means we need to refine the exception throwing in the existing streamTaskFile as well.

Contributor

kfaraz Aug 2, 2023

Yeah, you can modify the exception if required, as long as the overall logic remains the same. The advantage of having a separate retry method is readability and a small diff.

kfaraz reviewed

View reviewed changes

extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3TaskLogs.java Outdated

@@ @@ -67,6 +67,11 @@ public S3TaskLogs( @@
                 public Optional<InputStream> streamTaskLog(final String taskid, final long offset) throws IOException
                 {
                   final String taskKey = getTaskLogKey(taskid, "log");
+                  // this is to satisfy CodeQL scan
+                  Preconditions.checkArgument(
+                      offset < Long.MAX_VALUE && offset > Long.MIN_VALUE,

Contributor

kfaraz Aug 2, 2023

I think we can ignore the CodeQL scan for now because I am not entirely sure if Long.MIN_VALUE or Long.MAX_VALUE are not being used anywhere on purpose to represent some special scenarios.

The scan might also go away if we make the suggested change of adding a new method rather than updating the existing one. Although, I am not entirely sure if it will.

Contributor Author

YongGang Aug 2, 2023

ok, though PR can't be merged with CodeQL scan issue right?

kfaraz reviewed

View reviewed changes

Contributor

kfaraz left a comment

Changes look good, left some minor comments.

kfaraz added the Area - Ingestion label


          introduce a new retry method

481d6f5

github-advanced-security bot found potential problems

View reviewed changes

extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3TaskLogs.java Fixed Show fixed Hide fixed

YongGang added 2 commits

August 2, 2023 11:37


          satisfy CodeQL scan

6be48a4


          fix CodeQL scan

9e3447c

kfaraz approved these changes

View reviewed changes

Contributor

kfaraz left a comment

Minor suggestions, otherwise looks good.

extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3TaskLogs.java Outdated Show resolved Hide resolved

extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3TaskLogs.java Outdated Show resolved Hide resolved

extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3TaskLogsTest.java Outdated

Comment on lines 494 to 511

+                  EasyMock.reset(s3Client);
+                  AmazonS3Exception awsError = new AmazonS3Exception("AWS Error");
+                  awsError.setErrorCode("503");
+                  awsError.setStatusCode(503);
+                  EasyMock.expect(s3Client.getObjectMetadata(EasyMock.anyString(), EasyMock.anyString())).andThrow(awsError);
+                  EasyMock.expectLastCall().once();
+                  String logPath = TEST_PREFIX + "/" + KEY_1 + "/status.json";
+                  ObjectMetadata objectMetadata = new ObjectMetadata();
+                  objectMetadata.setContentLength(STATUS_CONTENTS.length());
+                  EasyMock.expect(s3Client.getObjectMetadata(TEST_BUCKET, logPath)).andReturn(objectMetadata);
+                  S3Object s3Object = new S3Object();
+                  s3Object.setObjectContent(new ByteArrayInputStream(STATUS_CONTENTS.getBytes(StandardCharsets.UTF_8)));
+                  GetObjectRequest getObjectRequest = new GetObjectRequest(TEST_BUCKET, logPath);
+                  getObjectRequest.setRange(0, STATUS_CONTENTS.length() - 1);
+                  getObjectRequest.withMatchingETagConstraint(objectMetadata.getETag());
+                  EasyMock.expect(s3Client.getObject(getObjectRequest)).andReturn(s3Object);
+                  EasyMock.expectLastCall().once();
+                  replayAll();

Contributor

kfaraz Aug 3, 2023

Some 1-line comments might be good here, or at least a logical separation using newlines.

YongGang and others added 3 commits

August 2, 2023 21:38


          Update extensions-core/s3-extensions/src/main/java/org/apache/druid/s…

89864fd

…torage/s3/S3TaskLogs.java

Co-authored-by: Kashif Faraz <[email protected]>


          Update extensions-core/s3-extensions/src/main/java/org/apache/druid/s…

744985a

…torage/s3/S3TaskLogs.java

Co-authored-by: Kashif Faraz <[email protected]>


          add comment to UT

d055767

georgew5656 approved these changes

View reviewed changes

kfaraz merged commit 20c48b6 into apache:master

YongGang deleted the retry-s3-task-status branch

August 17, 2023 16:02

LakshSingla added this to the 28.0 milestone

LakshSingla mentioned this pull request

[DRAFT] 28.0.0 release notes #15326

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area - Ingestion