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.

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

fix: Make the one breaking change required to allow regions on TopicPath or SubscriptionPath #729

Merged
merged 2 commits into from Jun 24, 2021
Merged

fix: Make the one breaking change required to allow regions on TopicPath or SubscriptionPath #729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0266f3c
fix: Make the one breaking change required to allow regions on TopicP…
dpcollins-google Jun 24, 2021
12c6a3c
fix: Code should use "extractRegion" which is the location-ambiguous …
dpcollins-google Jun 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 2 additions & 14 deletions google-cloud-pubsublite/clirr-ignored-differences.xml
View file
Open in desktop
Expand Up @@ -4,28 +4,16 @@
<!-- TODO: Remove on next release -->
<difference>
<differenceType>7006</differenceType>
<className>com/google/cloud/pubsublite/LocationPath</className>
<className>com/google/cloud/pubsublite/SubscriptionPath</className>
<method>*</method>
<to>*</to>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/pubsublite/AdminClient</className>
<method>*</method>
</difference>
<difference>
<!-- This one is actually a clirr bug: It doesn't handle CRTP well. -->
<differenceType>7006</differenceType>
<className>com/google/cloud/pubsublite/LocationPath$Builder</className>
<className>com/google/cloud/pubsublite/TopicPath</className>
<method>*</method>
<to>*</to>
</difference>
<difference>
<!-- This one is actually a clirr bug: It doesn't handle CRTP well. -->
<differenceType>7002</differenceType>
<className>com/google/cloud/pubsublite/LocationPath$Builder</className>
<method>*</method>
</difference>
<!-- END TODO: Remove on next release -->
<!-- Added abstract method to AutoValue.Builder class (Always okay) -->
<difference>
Expand Down
6 changes: 4 additions & 2 deletions google-cloud-pubsublite/src/main/java/com/google/cloud/pubsublite/PartitionLookupUtils.java
View file
Open in desktop
Expand Up @@ -31,7 +31,7 @@ private PartitionLookupUtils() {}
public static int numPartitions(TopicPath topic) throws ApiException {
try (AdminClient client =
AdminClient.create(
AdminClientSettings.newBuilder().setRegion(topic.location().region()).build())) {
AdminClientSettings.newBuilder().setRegion(topic.location().extractRegion()).build())) {
return numPartitions(topic, client);
} catch (Exception e) {
throw ExtractStatus.toCanonical(e).underlying;
Expand Down Expand Up @@ -59,7 +59,9 @@ public static int numPartitions(TopicPath topic, AdminClient client) throws ApiE
public static int numPartitions(SubscriptionPath subscription) throws ApiException {
try (AdminClient client =
AdminClient.create(
AdminClientSettings.newBuilder().setRegion(subscription.location().region()).build())) {
AdminClientSettings.newBuilder()
.setRegion(subscription.location().extractRegion())
.build())) {
return numPartitions(subscription, client);
} catch (Throwable t) {
throw ExtractStatus.toCanonical(t).underlying;
Expand Down
12 changes: 9 additions & 3 deletions google-cloud-pubsublite/src/main/java/com/google/cloud/pubsublite/SubscriptionPath.java
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@
public abstract class SubscriptionPath implements Serializable {
public abstract ProjectIdOrNumber project();

public abstract CloudZone location();
public abstract CloudRegionOrZone location();

public abstract SubscriptionName name();

Expand All @@ -55,7 +55,12 @@ public static Builder newBuilder() {

@AutoValue.Builder
public abstract static class Builder extends ProjectBuilderHelper<Builder> {
public abstract Builder setLocation(CloudZone zone);
// TODO(dpcollins): Make this public and use ProjectLocationBuilderHelper once region is allowed
abstract Builder setLocation(CloudRegionOrZone location);

public Builder setLocation(CloudZone zone) {
return setLocation(CloudRegionOrZone.of(zone));
}

public abstract Builder setName(SubscriptionName name);

Expand All @@ -68,12 +73,13 @@ public static SubscriptionPath parse(String path) throws ApiException {
checkArgument(splits.length == 6);
checkArgument(splits[4].equals("subscriptions"));
LocationPath location = LocationPath.parse(String.join("/", Arrays.copyOf(splits, 4)));
// TODO(dpcollins): Remove once region is allowed
checkArgument(
location.location().getKind() == Kind.ZONE,
"Subscription location must be a valid cloud zone.");
return SubscriptionPath.newBuilder()
.setProject(location.project())
.setLocation(location.location().zone())
.setLocation(location.location())
.setName(SubscriptionName.of(splits[5]))
.build();
}
Expand Down
12 changes: 9 additions & 3 deletions google-cloud-pubsublite/src/main/java/com/google/cloud/pubsublite/TopicPath.java
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@
public abstract class TopicPath implements Serializable {
public abstract ProjectIdOrNumber project();

public abstract CloudZone location();
public abstract CloudRegionOrZone location();

public abstract TopicName name();

Expand All @@ -55,7 +55,12 @@ public static Builder newBuilder() {

@AutoValue.Builder
public abstract static class Builder extends ProjectBuilderHelper<Builder> {
public abstract Builder setLocation(CloudZone zone);
// TODO(dpcollins): Make this public and use ProjectLocationBuilderHelper once region is allowed
abstract Builder setLocation(CloudRegionOrZone location);

public Builder setLocation(CloudZone zone) {
return setLocation(CloudRegionOrZone.of(zone));
}

public abstract Builder setName(TopicName name);

Expand All @@ -68,11 +73,12 @@ public static TopicPath parse(String path) throws ApiException {
checkArgument(splits.length == 6);
checkArgument(splits[4].equals("topics"));
LocationPath location = LocationPath.parse(String.join("/", Arrays.copyOf(splits, 4)));
// TODO(dpcollins): Remove once region is allowed
checkArgument(
location.location().getKind() == Kind.ZONE, "Topic location must be a valid cloud zone.");
return TopicPath.newBuilder()
.setProject(location.project())
.setLocation(location.location().zone())
.setLocation(location.location())
.setName(TopicName.of(splits[5]))
.build();
}
Expand Down
6 changes: 3 additions & 3 deletions ...d-pubsublite/src/main/java/com/google/cloud/pubsublite/cloudpubsub/PublisherSettings.java
View file
Open in desktop
Expand Up @@ -158,7 +158,7 @@ private PublisherServiceClient newServiceClient(Partition partition) throws ApiE
settingsBuilder);
try {
return PublisherServiceClient.create(
addDefaultSettings(topicPath().location().region(), settingsBuilder));
addDefaultSettings(topicPath().location().extractRegion(), settingsBuilder));
} catch (Throwable t) {
throw toCanonical(t).underlying;
}
Expand All @@ -172,10 +172,10 @@ private AdminClient getAdminClient() throws ApiException {
.setServiceClient(
AdminServiceClient.create(
addDefaultSettings(
topicPath().location().region(),
topicPath().location().extractRegion(),
AdminServiceSettings.newBuilder()
.setCredentialsProvider(credentialsProvider()))))
.setRegion(topicPath().location().region())
.setRegion(topicPath().location().extractRegion())
.build());
} catch (Throwable t) {
throw toCanonical(t).underlying;
Expand Down
6 changes: 3 additions & 3 deletions ...-pubsublite/src/main/java/com/google/cloud/pubsublite/cloudpubsub/SubscriberSettings.java
View file
Open in desktop
Expand Up @@ -220,7 +220,7 @@ private SubscriberServiceClient newSubscriberServiceClient(Partition partition)
RoutingMetadata.of(subscriptionPath(), partition),
settingsBuilder);
return SubscriberServiceClient.create(
addDefaultSettings(subscriptionPath().location().region(), settingsBuilder));
addDefaultSettings(subscriptionPath().location().extractRegion(), settingsBuilder));
} catch (Throwable t) {
throw toCanonical(t).underlying;
}
Expand All @@ -233,7 +233,7 @@ private CursorServiceClient newCursorServiceClient() throws ApiException {
try {
return CursorServiceClient.create(
addDefaultSettings(
subscriptionPath().location().region(),
subscriptionPath().location().extractRegion(),
CursorServiceSettings.newBuilder().setCredentialsProvider(credentialsProvider())));
} catch (Throwable t) {
throw toCanonical(t).underlying;
Expand Down Expand Up @@ -282,7 +282,7 @@ private PartitionAssignmentServiceClient getAssignmentServiceClient() throws Api
try {
return PartitionAssignmentServiceClient.create(
addDefaultSettings(
subscriptionPath().location().region(),
subscriptionPath().location().extractRegion(),
PartitionAssignmentServiceSettings.newBuilder()
.setCredentialsProvider(credentialsProvider())));
} catch (Throwable t) {
Expand Down
6 changes: 3 additions & 3 deletions pubsublite-beam-io/src/main/java/com/google/cloud/pubsublite/beam/Publishers.java
View file
Open in desktop
Expand Up @@ -51,9 +51,9 @@ private static AdminClient newAdminClient(PublisherOptions options) throws ApiEx
.setServiceClient(
AdminServiceClient.create(
addDefaultSettings(
options.topicPath().location().region(),
options.topicPath().location().extractRegion(),
AdminServiceSettings.newBuilder())))
.setRegion(options.topicPath().location().region())
.setRegion(options.topicPath().location().extractRegion())
.build());
} catch (Throwable t) {
throw toCanonical(t).underlying;
Expand All @@ -70,7 +70,7 @@ private static PublisherServiceClient newServiceClient(
settingsBuilder);
try {
return PublisherServiceClient.create(
addDefaultSettings(options.topicPath().location().region(), settingsBuilder));
addDefaultSettings(options.topicPath().location().extractRegion(), settingsBuilder));
} catch (Throwable t) {
throw toCanonical(t).underlying;
}
Expand Down
2 changes: 1 addition & 1 deletion pubsublite-beam-io/src/main/java/com/google/cloud/pubsublite/beam/SubscribeTransform.java
View file
Open in desktop
Expand Up @@ -114,7 +114,7 @@ private TopicPath getTopicPath() {
try (AdminClient admin =
AdminClient.create(
AdminClientSettings.newBuilder()
.setRegion(options.subscriptionPath().location().region())
.setRegion(options.subscriptionPath().location().extractRegion())
.build())) {
return TopicPath.parse(admin.getSubscription(options.subscriptionPath()).get().getTopic());
} catch (Throwable t) {
Expand Down
6 changes: 3 additions & 3 deletions pubsublite-beam-io/src/main/java/com/google/cloud/pubsublite/beam/SubscriberOptions.java
View file
Open in desktop
Expand Up @@ -119,7 +119,7 @@ private SubscriberServiceClient newSubscriberServiceClient(Partition partition)
RoutingMetadata.of(subscriptionPath(), partition),
settingsBuilder);
return SubscriberServiceClient.create(
addDefaultSettings(subscriptionPath().location().region(), settingsBuilder));
addDefaultSettings(subscriptionPath().location().extractRegion(), settingsBuilder));
} catch (Throwable t) {
throw toCanonical(t).underlying;
}
Expand Down Expand Up @@ -147,7 +147,7 @@ private CursorServiceClient newCursorServiceClient() throws ApiException {
try {
return CursorServiceClient.create(
addDefaultSettings(
subscriptionPath().location().region(), CursorServiceSettings.newBuilder()));
subscriptionPath().location().extractRegion(), CursorServiceSettings.newBuilder()));
} catch (Throwable t) {
throw toCanonical(t).underlying;
}
Expand Down Expand Up @@ -186,7 +186,7 @@ InitialOffsetReader getInitialOffsetReader(Partition partition) {
return new InitialOffsetReaderImpl(
CursorClient.create(
CursorClientSettings.newBuilder()
.setRegion(subscriptionPath().location().region())
.setRegion(subscriptionPath().location().extractRegion())
.build()),
subscriptionPath(),
partition);
Expand Down
6 changes: 4 additions & 2 deletions ...te-beam-io/src/main/java/com/google/cloud/pubsublite/beam/TopicBacklogReaderSettings.java
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ Builder setTopicPathFromSubscriptionPath(SubscriptionPath subscriptionPath)
try (AdminClient adminClient =
AdminClient.create(
AdminClientSettings.newBuilder()
.setRegion(subscriptionPath.location().region())
.setRegion(subscriptionPath.location().extractRegion())
.build())) {
return setTopicPath(
TopicPath.parse(adminClient.getSubscription(subscriptionPath).get().getTopic()));
Expand All @@ -80,7 +80,9 @@ Builder setTopicPathFromSubscriptionPath(SubscriptionPath subscriptionPath)

TopicBacklogReader instantiate() throws ApiException {
TopicStatsClientSettings settings =
TopicStatsClientSettings.newBuilder().setRegion(topicPath().location().region()).build();
TopicStatsClientSettings.newBuilder()
.setRegion(topicPath().location().extractRegion())
.build();
TopicBacklogReader impl =
new TopicBacklogReaderImpl(TopicStatsClient.create(settings), topicPath(), partition());
return new LimitingTopicBacklogReader(impl, Ticker.systemTicker());
Expand Down