Adding original message to the throwble for Pub/Sub publish failures. #2020

sateekot · 2019-11-07T10:13:29Z

Adding original message to Future provided by Publisher if any failures to publish events to Pub/Sub.

pivotal-issuemaster · 2019-11-07T10:13:31Z

@sateekot Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2019-11-07T11:01:45Z

@sateekot Thank you for signing the Contributor License Agreement!

codecov · 2019-11-07T11:01:49Z

Codecov Report

Merging #2020 into master will decrease coverage by 7.95%.
The diff coverage is 87.5%.

@@             Coverage Diff              @@
##             master    #2020      +/-   ##
============================================
- Coverage     80.44%   72.48%   -7.96%     
+ Complexity     2120     1885     -235     
============================================
  Files           243      244       +1     
  Lines          6907     6913       +6     
  Branches        710      710              
============================================
- Hits           5556     5011     -545     
- Misses         1056     1574     +518     
- Partials        295      328      +33

Flag	Coverage Δ	Complexity Δ
#integration	`?`	`?`
#unittests	`72.48% <87.5%> (ø)`	`1885 <1> (+1)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...pubsub/core/publisher/PubSubPublisherTemplate.java	`86.2% <100%> (+1.02%)`	`6 <0> (ø)`	⬇️
...cloud/gcp/pubsub/core/PubSubDeliveryException.java	`75% <75%> (ø)`	`1 <1> (?)`
...a/spanner/repository/query/SpannerQueryMethod.java	`0% <0%> (-100%)`	`0% <0%> (-6%)`
...ository/config/FirestoreRepositoriesRegistrar.java	`0% <0%> (-100%)`	`0% <0%> (-3%)`
...sitory/support/FirestoreRepositoryFactoryBean.java	`0% <0%> (-100%)`	`0% <0%> (-4%)`
...pository/support/FirestoreQueryLookupStrategy.java	`0% <0%> (-100%)`	`0% <0%> (-2%)`
...figure/config/GcpConfigBootstrapConfiguration.java	`0% <0%> (-100%)`	`0% <0%> (-2%)`
...restore/repository/query/FirestoreQueryMethod.java	`0% <0%> (-100%)`	`0% <0%> (-2%)`
...ository/config/DatastoreRepositoriesRegistrar.java	`0% <0%> (-100%)`	`0% <0%> (-3%)`
...epository/config/SpannerRepositoriesRegistrar.java	`0% <0%> (-100%)`	`0% <0%> (-3%)`
... and 54 more

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 13d06ff...a1f9442. Read the comment docs.

meltsufin

Thank you for putting together the PR!
I just have one suggested change.
BTW this is for #2003

meltsufin · 2019-11-07T20:42:38Z

...c/main/java/org/springframework/cloud/gcp/pubsub/core/publisher/PubSubPublisherTemplate.java

@@ -103,7 +106,10 @@ public void setMessageConverter(PubSubMessageConverter pubSubMessageConverter) {
 			@Override
 			public void onFailure(Throwable throwable) {
 				LOGGER.warn("Publishing to " + topic + " topic failed.", throwable);
-				settableFuture.setException(throwable);
+				Message<?> originalMessage = MessageBuilder.withPayload(pubsubMessage).build();
+				MessageHandlingException messageHandlingException = new MessageHandlingException(


Can you use MessageDeliveryException instead please?

Well, I wouldn't do that.
I mean we are good to have a PubSubDeliveryException as an extension of the PubSubException.
My point is to not wrap a PubsubMessage into a Message<?>: an original request is not Spring Messaging.
So, let's not abuse non-related API, but we definitely can borrow an idea! 😄

Thank you for your comments. I have updated the code accordingly. Can you please check the updated commit.

artembilan

Your name, please, into the modified classes.
And let's consider to mention such a feature in docs somehow!

Thanks

artembilan · 2019-11-07T21:11:24Z

...c/main/java/org/springframework/cloud/gcp/pubsub/core/publisher/PubSubPublisherTemplate.java

@@ -103,7 +106,10 @@ public void setMessageConverter(PubSubMessageConverter pubSubMessageConverter) {
 			@Override
 			public void onFailure(Throwable throwable) {
 				LOGGER.warn("Publishing to " + topic + " topic failed.", throwable);
-				settableFuture.setException(throwable);
+				Message<?> originalMessage = MessageBuilder.withPayload(pubsubMessage).build();
+				MessageHandlingException messageHandlingException = new MessageHandlingException(


Well, I wouldn't do that.
I mean we are good to have a PubSubDeliveryException as an extension of the PubSubException.
My point is to not wrap a PubsubMessage into a Message<?>: an original request is not Spring Messaging.
So, let's not abuse non-related API, but we definitely can borrow an idea! 😄

sateekot · 2019-11-08T12:17:15Z

@artembilan I am not sure how to update this feature in docs, Can you please give me some pointers.

meltsufin

The docs could be updated in pubsub.adoc. If you don't feel comfortable doing it, it's ok to create an issue to do it in a separate PR.

meltsufin · 2019-11-08T14:31:32Z

...-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/core/PubSubDeliveryException.java

+		this.failedMessage = pubsubMessage;
+	}
+
+	public PubSubDeliveryException(PubsubMessage pubsubMessage, Throwable cause) {


Let's just keep the one constructor we currently use. We can always add more. Otherwise, we have all this code that's missing coverage.

removed constructors.

meltsufin · 2019-11-08T14:34:09Z

...-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/core/PubSubDeliveryException.java

+	}
+
+	@Override
+	public String toString() {


We don't typically overwrite the toString() for exceptions. Who is going to call this method?

If we don't override toString(), the newly added test case is failing as we are checking the message in the exception.

That has to be done with the cast and that getFailedMessage().

So, yes +1 to remove this toString()

artembilan

Here is some review from me.

Nothing critical - a general solution is OK.

artembilan · 2019-11-08T16:57:11Z

...-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/core/PubSubDeliveryException.java

+	}
+
+	@Override
+	public String toString() {


That has to be done with the cast and that getFailedMessage().

So, yes +1 to remove this toString()

artembilan · 2019-11-08T16:58:12Z

...-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/core/PubSubDeliveryException.java

+	private final PubsubMessage failedMessage;
+
+	public PubSubDeliveryException(PubsubMessage pubsubMessage, Throwable cause) {
+		super(null, cause);


I think we need to provide some message into an exception anyway.
I mean that required by super class description.
therefore one more ctor arg in this PubSubDeliveryException

artembilan · 2019-11-08T17:00:29Z

...-gcp-pubsub/src/test/java/org/springframework/cloud/gcp/pubsub/core/PubSubTemplateTests.java

+	public void testPublish_onFailureWithPayload() {
+		ListenableFuture<String> future = this.pubSubTemplate.publish("testTopic", this.pubsubMessage);
+		this.settableApiFuture.setException(new Exception("Publish failed"));
+		assertThatThrownBy(() -> future.get()).isInstanceOf(ExecutionException.class).hasMessageContaining("permanating")


I think we need to check in the test that we have already that new PubSubDeliveryException with expected this.pubsubMessage populated. I believe that was a main goal of this fix.

artembilan · 2019-11-11T15:14:12Z

...c/main/java/org/springframework/cloud/gcp/pubsub/core/publisher/PubSubPublisherTemplate.java

@@ -103,7 +105,8 @@ public void setMessageConverter(PubSubMessageConverter pubSubMessageConverter) {
 			@Override
 			public void onFailure(Throwable throwable) {
 				LOGGER.warn("Publishing to " + topic + " topic failed.", throwable);
-				settableFuture.setException(throwable);
+				PubSubDeliveryException pubSubDeliveryException = new PubSubDeliveryException(pubsubMessage, throwable.getMessage(), throwable);


No, I don't want to see the same throwable.getMessage() here again on top of stack trace.
I can get it in the stack trace anyway parsing a cause.
What I want to see is an explicit message indicating that exactly here in the PubSubTemplate we couldn't delivered a message.

+1
I think what you're logging should work here as well for the message:
"Publishing to " + topic + " topic failed."

Updated the code.

meltsufin · 2019-11-11T15:37:15Z

...c/main/java/org/springframework/cloud/gcp/pubsub/core/publisher/PubSubPublisherTemplate.java

@@ -103,7 +105,8 @@ public void setMessageConverter(PubSubMessageConverter pubSubMessageConverter) {
 			@Override
 			public void onFailure(Throwable throwable) {
 				LOGGER.warn("Publishing to " + topic + " topic failed.", throwable);
-				settableFuture.setException(throwable);
+				PubSubDeliveryException pubSubDeliveryException = new PubSubDeliveryException(pubsubMessage, throwable.getMessage(), throwable);


+1
I think what you're logging should work here as well for the message:
"Publishing to " + topic + " topic failed."

Adding original message to the throwble for Pub/Sub publish failures.

30809c8

viniciusccarvalho mentioned this pull request Nov 7, 2019

Add original event details in CallbackListener (ListenableFutureCallback<String>). #2003

Closed

meltsufin suggested changes Nov 7, 2019

View reviewed changes

artembilan suggested changes Nov 7, 2019

View reviewed changes

Incorporating review comment changes for Issue 2003.

fc2e6f5

meltsufin suggested changes Nov 8, 2019

View reviewed changes

skota-quot added 2 commits November 8, 2019 20:59

Changes incorporated.

6cd0232

Changes incorporated.

4801d55

artembilan suggested changes Nov 8, 2019

View reviewed changes

Updating changes as per comments.

87f22aa

artembilan suggested changes Nov 11, 2019

View reviewed changes

meltsufin added the pubsub GCP PubSub label Nov 11, 2019

meltsufin suggested changes Nov 11, 2019

View reviewed changes

Changes incorporated.

a1f9442

artembilan approved these changes Nov 11, 2019

View reviewed changes

meltsufin approved these changes Nov 11, 2019

View reviewed changes

meltsufin merged commit 07a316e into spring-attic:master Nov 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding original message to the throwble for Pub/Sub publish failures. #2020

Adding original message to the throwble for Pub/Sub publish failures. #2020

sateekot commented Nov 7, 2019

pivotal-issuemaster commented Nov 7, 2019

pivotal-issuemaster commented Nov 7, 2019

codecov bot commented Nov 7, 2019 •

edited

Loading

meltsufin left a comment

meltsufin Nov 7, 2019

artembilan Nov 7, 2019

sateekot Nov 8, 2019

artembilan left a comment

artembilan Nov 7, 2019

sateekot commented Nov 8, 2019

meltsufin left a comment

meltsufin Nov 8, 2019

sateekot Nov 8, 2019

meltsufin Nov 8, 2019

sateekot Nov 8, 2019

artembilan Nov 8, 2019

artembilan left a comment

artembilan Nov 8, 2019

artembilan Nov 8, 2019

artembilan Nov 8, 2019

artembilan Nov 11, 2019

meltsufin Nov 11, 2019

sateekot Nov 11, 2019

meltsufin Nov 11, 2019

Adding original message to the throwble for Pub/Sub publish failures. #2020

Adding original message to the throwble for Pub/Sub publish failures. #2020

Conversation

sateekot commented Nov 7, 2019

pivotal-issuemaster commented Nov 7, 2019

pivotal-issuemaster commented Nov 7, 2019

codecov bot commented Nov 7, 2019 • edited Loading

Codecov Report

meltsufin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sateekot commented Nov 8, 2019

meltsufin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 7, 2019 •

edited

Loading