- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
Upgrade to latest GCS SDK #124062
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.
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
Upgrade to latest GCS SDK #124062
Conversation
| identification within third-party archives. | ||
| 
               | 
          ||
| Copyright 1999-2005 The Apache Software Foundation | ||
| Copyright [yyyy] [name of copyright owner] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really the LICENSE: https://github.com/open-telemetry/opentelemetry-java/blob/main/LICENSE
| exchange.getResponseHeaders().add("Range", updateResponse.rangeHeader().headerString()); | ||
| } | ||
| exchange.getResponseHeaders().add("Content-Length", "0"); | ||
| exchange.getResponseHeaders().add("x-goog-stored-content-length", String.valueOf(updateResponse.storedContentLength())); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new client is stricter about receiving a content-length
        
          
                ...est/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainerRetriesTests.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | 
           Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)  | 
    
| 
           Hi @nicktindall, I've created a changelog YAML for you.  | 
    
| 
           Hi @nicktindall, I've updated the changelog YAML for you.  | 
    
| throw storageException; | ||
| } | ||
| throw e; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative to this is something like
catch (IOException | StorageException e) {
    StorageException se = (StorageException) ExceptionHelper.unwrap(e, StorageException.class);
    if (se != null) {
        // do StorageException-specific things
    }
}in both catch blocks
| // Also retry on `SocketException`s | ||
| if (ExceptionsHelper.unwrap(prevThrowable, SocketException.class) != null) { | ||
| return true; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something has changed at the HTTP call sites and we now get BaseStorageExceptions propagate with cause SocketException and retryable=false. The legacy retry strategy relies on retryable flag so we need to manually intervene here.
I think this is a sign we should be making moves towards DefaultStorageRetryStrategy as I think it would be handled correctly there.
Actually that's worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This reverts commit 073ca0e.
This reverts commit 073ca0e.
This reverts commit 073ca0e.
This reverts commit 073ca0e.
Upgrades google cloud SDK used by repository-gcp to com.google.cloud:google-cloud-storage-bom:2.50.0 Closes: ES-9287
This reverts commit 073ca0e.
Upgraded to
com.google.cloud:google-cloud-storage-bom:2.50.0This is the first release that includes the fix for googleapis/java-storage#2972, which breaks our build because of the randomised locale.