Remove exception on failure response from GCS delete API by gargvishesh · Pull Request #16047 · apache/druid

gargvishesh · 2024-03-05T09:39:58Z

The previously used Google API Client library for Google Cloud Storage returned a 404 HTTPResponseException for delete when the object to be deleted was not found, and was propagated as-is by the Druid GCS extension. The currently used Google Cloud Client library just returns a false response in that scenario, which was being converted to a generic IOException in the extension -- breaking flows where file missing is an acceptable behaviour.

This PR removes the exception altogether since all current delete scenarios don't care if the file-to-be-deleted is missing

Co-authored-by: Abhishek Radhakrishnan <abhishek.rb19@gmail.com>

abhishekagarwal87 · 2024-03-06T03:59:04Z

    if (!storage.get().delete(bucket, path)) {
-      throw new IOE(
-          "Failed deleting google cloud storage object [bucket: %s path: %s]",
+      log.warn(StringUtils.nonStrictFormat(


since it was debug previously, lets keep it at debug level

I understand this is not to spook existing users when they suddenly start seeing warn for missing descriptor.json. Else it better be visible to spot logic bugs.

abhishekagarwal87 · 2024-03-06T04:00:34Z

    List<Boolean> statuses = storage.get().delete(Iterables.transform(paths, input -> BlobId.of(bucket, input)));
    if (statuses.contains(false)) {
-      throw new IOE("Failed deleting google cloud storage object(s)");
+      log.warn("Google cloud storage object(s) to be deleted not found");


it can be debug level as well. Also please log the paths and bucket

Changed and included.

abhishekagarwal87 · 2024-03-06T04:05:32Z

      deleteIfPresent(bucket, descriptorPath);
    }
-    catch (IOException e) {
+    catch (StorageException e) {


Why this change?

deleteIfPresent no longer throws IOException

the underlying lib could still throw an IOException though?

Nope, it can't -- else it has to be caught or declared thrown since it's a checked exception.

I think we could add IOException too here so that any random IO error is bubbled up as SegmentLoadingException

Currently wIth retry, only 2 exception kinds are possible: a) Exception raised by the retried call (only StorageException in this case), and b) RuntimeException.

druid/processing/src/main/java/org/apache/druid/java/util/common/RetryUtils.java

Line 140 in bf39c71

Throwables.propagateIfInstanceOf(e, Exception.class);

Do we still want to include IOException handling?

Hmm. probably ok to leave.

* change runtime exception class for code coverage * Add file paths for batch delete failures

gargvishesh

Thank you @abhishekagarwal87 and @abhishekrb19 for the thorough reviews!

abhishekrb19

Thanks for addressing the comments swiftly, @gargvishesh! I finished my review of the tests and left a few comments along with some nits, but otherwise looks good.

* Throw 404 Exception on failure response from GCS delete API * Replace String.format * Apply suggestions from code review Co-authored-by: Abhishek Radhakrishnan <abhishek.rb19@gmail.com> * Remove exception for file not found and fix tests * Add warn log and fix intellij inspection errors * More intellij inspection fixes * * Change to debug log * change runtime exception class for code coverage * Add file paths for batch delete failures * Move failedPaths computation to inside isDebugEnabled flag * Correct handling of StorageException * Address review comments * Remove unused exceptions * Address code coverage and review comments * Minor corrections --------- Co-authored-by: Abhishek Radhakrishnan <abhishek.rb19@gmail.com> (cherry picked from commit bed5d9c)

…6095) * Throw 404 Exception on failure response from GCS delete API * Replace String.format * Apply suggestions from code review Co-authored-by: Abhishek Radhakrishnan <abhishek.rb19@gmail.com> * Remove exception for file not found and fix tests * Add warn log and fix intellij inspection errors * More intellij inspection fixes * * Change to debug log * change runtime exception class for code coverage * Add file paths for batch delete failures * Move failedPaths computation to inside isDebugEnabled flag * Correct handling of StorageException * Address review comments * Remove unused exceptions * Address code coverage and review comments * Minor corrections --------- Co-authored-by: Abhishek Radhakrishnan <abhishek.rb19@gmail.com> (cherry picked from commit bed5d9c)

cryptoe · 2024-03-12T17:06:03Z

Caused because of : #15398

gargvishesh added 2 commits March 5, 2024 14:51

Throw 404 Exception on failure response from GCS delete API

6fe3e40

Replace String.format

0808ceb

abhishekagarwal87 reviewed Mar 5, 2024

View reviewed changes

Comment thread ...ions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorage.java Outdated

abhishekagarwal87 added this to the 29.0.1 milestone Mar 5, 2024

abhishekrb19 reviewed Mar 5, 2024

View reviewed changes

gargvishesh and others added 2 commits March 5, 2024 16:18

Apply suggestions from code review

b2d86e1

Co-authored-by: Abhishek Radhakrishnan <abhishek.rb19@gmail.com>

Remove exception for file not found and fix tests

d0ed1cf

gargvishesh changed the title ~~Throw 404 Exception on failure response from GCS delete API~~ Remove exception on failure response from GCS delete API Mar 5, 2024

abhishekagarwal87 reviewed Mar 5, 2024

View reviewed changes

Comment thread ...ions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorage.java Outdated

gargvishesh added 2 commits March 5, 2024 22:27

Add warn log and fix intellij inspection errors

c95f39d

More intellij inspection fixes

081d431

abhishekagarwal87 reviewed Mar 6, 2024

View reviewed changes

* Change to debug log

37b9c95

* change runtime exception class for code coverage * Add file paths for batch delete failures

abhishekagarwal87 reviewed Mar 6, 2024

View reviewed changes

Comment thread ...ions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorage.java Outdated

Comment thread ...le-extensions/src/test/java/org/apache/druid/storage/google/GoogleDataSegmentKillerTest.java

Move failedPaths computation to inside isDebugEnabled flag

74a8fa0

abhishekrb19 reviewed Mar 6, 2024

View reviewed changes

gargvishesh added 2 commits March 6, 2024 17:02

Merge branch 'master' into 42663-gcp-404-for-file-missing-on-delete

ff64f12

Correct handling of StorageException

46efe8f

abhishekagarwal87 reviewed Mar 6, 2024

View reviewed changes

Comment thread ...-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleStorageTest.java Outdated

Comment thread ...ns-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleUtilsTest.java Outdated

Address review comments

537eb3b

gargvishesh commented Mar 6, 2024

View reviewed changes

Remove unused exceptions

731a420

abhishekrb19 reviewed Mar 6, 2024

View reviewed changes

abhishekrb19 approved these changes Mar 6, 2024

View reviewed changes

gargvishesh added 2 commits March 7, 2024 14:47

Address code coverage and review comments

922ef3d

Minor corrections

f8ecb01

abhishekagarwal87 approved these changes Mar 7, 2024

View reviewed changes

abhishekrb19 merged commit bed5d9c into apache:master Mar 7, 2024

gargvishesh mentioned this pull request Mar 8, 2024

[Backport] Remove exception on failure response from GCS delete API #16085

Closed

gargvishesh mentioned this pull request Mar 11, 2024

[Backport] Remove exception on failure response from GCS delete API #16095

Merged

Conversation

gargvishesh commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gargvishesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abhishekrb19 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cryptoe commented Mar 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gargvishesh commented Mar 5, 2024 •

edited

Loading