Skip to content
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

Add MSQ Durable Storage Connector for Google Cloud Storage and change current Google Cloud Storage client library #15398

Merged
merged 30 commits into from
Dec 14, 2023

Conversation

gargvishesh
Copy link
Contributor

@gargvishesh gargvishesh commented Nov 20, 2023

Description

The PR addresses 2 things:

{
// Continuation token is an index in the "objects" list.
final String continuationToken = getPageToken();
final int startIndex = continuationToken == null ? 0 : Integer.parseInt(continuationToken);
final int startIndex = pageToken == null ? 0 : Integer.parseInt(pageToken);

Check notice

Code scanning / CodeQL

Missing catch of NumberFormatException Note test

Potential uncaught 'java.lang.NumberFormatException'.
@gargvishesh gargvishesh changed the title 35053-gcs-durable-storage-connector Add MSQ Durable Connector for Google Cloud Storage and change existing Google Storage client library Nov 21, 2023
@gargvishesh gargvishesh changed the title Add MSQ Durable Connector for Google Cloud Storage and change existing Google Storage client library Add MSQ Durable Storage Connector for Google Cloud Storage and change current Google Cloud Storage client library Nov 21, 2023
@gargvishesh gargvishesh marked this pull request as ready for review November 21, 2023 16:35
Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes overall LGTM. Minor comments.

@@ -48,16 +48,11 @@
</dependency>

<dependency>
<groupId>com.google.apis</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to update :

<dependency>
<groupId>com.google.apis</groupId>
<artifactId>google-api-services-storage</artifactId>
<version>${com.google.apis.storage.version}</version>
<exclusions>
<exclusion>
<groupId>com.google.api-client</groupId>
<artifactId>google-api-client</artifactId>
</exclusion>
</exclusions>
</dependency>
as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed now.

pom.xml Outdated Show resolved Hide resolved
// list with prefix can return directories, but they should always end with `/`, ignore them.
// also skips empty objects.
if (!next.getName().endsWith("/") && next.getSize().signum() > 0) {
if (!next.getName().endsWith("/") && Long.signum(next.getSize()) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Why would size be negative ever ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more a check meant for != 0

import java.io.OutputStream;
import java.util.Iterator;

public class GoogleStorageConnector extends ChunkingStorageConnector<GoogleInputRange>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I donot see test cases for this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added for this and some other classes as well.

Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM.
Will merge post a green build.

&& Objects.equals(bucket, that.bucket)
&& Objects.equals(path, that.path);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Lets add a toString to this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@cryptoe cryptoe merged commit e43bb74 into apache:master Dec 14, 2023
83 checks passed
@cryptoe
Copy link
Contributor

cryptoe commented Dec 14, 2023

Thanks @gargvishesh for working on this.

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

Successfully merging this pull request may close these issues.

3 participants