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

feat: add TopicName #113

Merged
merged 11 commits into from Apr 1, 2020
Merged

feat: add TopicName #113

merged 11 commits into from Apr 1, 2020

Conversation

yihanzhen
Copy link
Contributor

@yihanzhen yihanzhen commented Mar 17, 2020

  • Regenerate using the latest generator and introduce the new design of multi-pattern resource name (TopicName) in this case.
  • Use TopicName on gapic surface where ProjectTopicName was used.
  • Added text replacements in synth.py to add back methods that take ProjectTopicName.
  • No breaking changes.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 17, 2020
@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #113 into master will decrease coverage by 0.99%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master     #113    +/-   ##
==========================================
- Coverage     79.36%   78.37%    -1%     
+ Complexity      305      302     -3     
==========================================
  Files            21       21            
  Lines          2603     2719   +116     
  Branches        115      134    +19     
==========================================
+ Hits           2066     2131    +65     
- Misses          467      513    +46     
- Partials         70       75     +5
Impacted Files Coverage Δ Complexity Δ
...a/com/google/cloud/pubsub/v1/TopicAdminClient.java 52.05% <13.04%> (-10.91%) 29 <3> (-3)
...oogle/cloud/pubsub/v1/SubscriptionAdminClient.java 57.47% <15.78%> (-6.3%) 49 <3> (-3)
...ain/java/com/google/cloud/pubsub/v1/Publisher.java 87.96% <0%> (-1.14%) 46% <0%> (+3%)
...gle/cloud/pubsub/v1/SequentialExecutorService.java 91.01% <0%> (+0.2%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e2ee4f...ed26613. Read the comment docs.

@yihanzhen
Copy link
Contributor Author

Hey @chingor13 @kamalaboulhosn could you help with the integration tests?

I got:

[INFO] Running com.google.cloud.pubsub.it.ITPubSubTest
projects/gcloud-devel/topics/testing-topic-policy-7f56882e-3189-4447-bf99-83dd97c00997
projects/gcloud-devel/topics/testing-topic-policy-7f56882e-3189-4447-bf99-83dd97c00997
[ERROR] Tests run: 3, Failures: 0, Errors: 1, Skipped: 1, Time elapsed: 10.165 s <<< FAILURE! - in com.google.cloud.pubsub.it.ITPubSubTest
[ERROR] com.google.cloud.pubsub.it.ITPubSubTest.testTopicPolicy  Time elapsed: 2.672 s  <<< ERROR!
com.google.api.gax.rpc.PermissionDeniedException: io.grpc.StatusRuntimeException: PERMISSION_DENIED: User not authorized to perform this action.
	at com.google.api.gax.rpc.ApiExceptionFactory.createException(ApiExceptionFactory.java:55)
	at com.google.api.gax.grpc.GrpcApiExceptionFactory.create(GrpcApiExceptionFactory.java:72)
	at com.google.api.gax.grpc.GrpcApiExceptionFactory.create(GrpcApiExceptionFactory.java:60)
	at com.google.api.gax.grpc.GrpcExceptionCallable$ExceptionTransformingFuture.onFailure(GrpcExceptionCallable.java:97)
	at com.google.api.core.ApiFutures$1.onFailure(ApiFutures.java:68)
	at com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1039)
	at com.google.common.util.concurrent.DirectExecutor.execute(DirectExecutor.java:30)
	at com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:1165)
	at com.google.common.util.concurrent.AbstractFuture.complete(AbstractFuture.java:958)
	at com.google.common.util.concurrent.AbstractFuture.setException(AbstractFuture.java:749)
	at io.grpc.stub.ClientCalls$GrpcFuture.setException(ClientCalls.java:522)
	at io.grpc.stub.ClientCalls$UnaryStreamToFuture.onClose(ClientCalls.java:497)
	at io.grpc.internal.ClientCallImpl.closeObserver(ClientCallImpl.java:426)
	at io.grpc.internal.ClientCallImpl.access$500(ClientCallImpl.java:66)
	at io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl.close(ClientCallImpl.java:689)
	at io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl.access$900(ClientCallImpl.java:577)
	at io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl$1StreamClosed.runInternal(ClientCallImpl.java:751)
	at io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl$1StreamClosed.runInContext(ClientCallImpl.java:740)
	at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37)
	at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:123)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:180)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:293)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

Also what is the samples test for?

@kamalaboulhosn kamalaboulhosn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 20, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 20, 2020
@@ -360,6 +361,21 @@ public final Subscription createSubscription(Subscription request) {
return createSubscriptionCallable().call(request);
}

public final Subscription createSubscription(
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation? For this and any of the other public methods.

Copy link
Contributor Author

@yihanzhen yihanzhen Mar 24, 2020

Choose a reason for hiding this comment

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

So this method is only added back for binary backward-compatibility. The functionality of this method is already covered by createSubscription(TopicName topic), as the old ProjectTopicName is the subclass of the new TopicName.

We are trying to remove the per-pattern resource names (ProjectTopicName in this case) across all cloud APIs because as we scale it's harder and harder for the generator to come up with meaningful class names. Because PubSub doesn't want a major version bump so we injected code back to the generated clients as a postprocessing step.

So far for such cases we've been only adding essentials (code) back but leaving examples and documentation for all APIs (example). It will be doable to inject the documentation and comments back but will be adding lots of pain for future maintenance. Since I think it'll be PubSub team to continue to maintain the library I want to confirm that you really want me to add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should at least have a deprecated notice on this method, indicating the preference for the other createSubscription method and that this one will be removed in the next major version release. I think we do still have to document the public interface as whether or not it is being written manually or not, we need to ensure things are properly documented for our customers.

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. Both deprecation information and documentation is added.

vam-google added a commit to vam-google/java-pubsub that referenced this pull request Mar 30, 2020
This PR migrates only synth.py but does not commit the regenerated files. The generation was tested and it works, the updated files are not commited due to breaking changes not related to bazel migration.

This PR should be pushed after the an already open PR with the braking changes: googleapis#113
@yihanzhen
Copy link
Contributor Author

@kamalaboulhosn PTAL

@kamalaboulhosn kamalaboulhosn merged commit 4558c34 into googleapis:master Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants