PublisherFactory returns PublisherInterface instead of Publisher

[mlvandijk]

Describe the bug
There was a change in the PublisherFactory in com.google.cloud:spring-cloud-gcp-pubsub version 2.0.6 used in com.google.cloud:spring-cloud-gcp-starter-pubsub which caused one of our applications to have a compilation error on the following line:

pubsubTemplate.getPublisherFactory.createPublisher(topic).publishAllOutstanding()

The reason for this is as follows:

In version 2.0.5 the PublisherFactory returns a Publisher

public interface PublisherFactory {
    Publisher createPublisher(String topic);
}

while in v 2.0.6 it returns a PublisherInterface

public interface PublisherFactory {
    PublisherInterface createPublisher(String topic);
}

The PublisherInterface doesn't expose the Publisher method publishAllOutstanding().

This change is not mentioned in the change log for (release 2.0.6)[https://github.com/GoogleCloudPlatform/spring-cloud-gcp/releases/tag/v2.0.6].

The change was part of PR #548.
In this change I notice that the CachingPublisherFactory implementation of PublisherFactory was also change to return a PublisherInterface instead of a Publisher, but the DefaultPublisherFactory was not (and still returns a Publisher).

Since this isn't mentioned in the release notes, I wonder if this change was intentional? And also whether we can fix our compilation error by casting to a DefaultPublisherFactory going forward, or should assume that the publishAllOutstanding() is not intended to be exposed (as it's not part of the PublisherInterface).

Update: casting to DefaultPublisherFactory didn't work.

[mlvandijk]

Should have label "pubsub" but I cannot add labels.

[mlvandijk]

Additional info: Downgrading to 2.0.5 fixes the compilation error but the deployment fails. There is an issue with a class not found:
java.lang.IllegalArgumentException: Unresolvable class definition for class [com.google.cloud.spring.autoconfigure.pubsub.GcpPubSubProperties]

[elefeint]

@mlvandijk For the second issue, you'd have to also downgrade spring-cloud-gcp-autoconfigure to 2.0.5.
For the main issue, you are absolutely correct -- this was an unintentional regression.

@meltsufin If we make TracingPublisher extend Publisher for backwards compatibility while retaining the delegate, we'd have to override all methods to redirect to the composed delegate. It might be easier to avoid future breakage, if TracingPublisher extends Publisher, removes the delegate and only overrides publish(). Any better ideas?

[meltsufin]

@mlvandijk Can you just downcast PublisherInterface to Publisher, if you're not using Pub/Sub tracing?

@elefeint Your suggestion makes sense, but it will require quite a bit of boilerplate code. Ideally, PublisherInterface would expose publishAllOutstanding(), but that would require changes in the the Pub/Sub client library.

[mlvandijk]

Thank you both for your quick response. I will see what we can do today to work around it and get back to you.

[mlvandijk]

Update: Casting PublisherInterface to Publisher seems to work.
pubsubTemplate.getPublisherFactory.createPublisher(topic).asInstanceOf[Publisher].publishAllOutstanding() (we are using Scala in this part of the application)

[elefeint]

I've filed googleapis/java-pubsub#949 in the client library -- if it's meant to be a public surface, we should support it. If not, the downcasting workaround is fine.

[mlvandijk]

Update: We are still having trouble with our application so we had to roll back (with the downcast). We are still investigating to pinpoint the issue. If relevant, I will provide additional info here.

[elefeint]

That's interesting. What type of issues (memory? throughput?). And are the problems coming up in the "publish to pub/sub" scenario or elsewhere?

[mlvandijk]

We noticed that we were no longer processing messages, and our k8s pods kept crashing.

For context: We have several applications that process large amounts of data. Two of them seem to have a problem with the upgrade from spring-cloud-gcp-pubsub version 2.0.5 -> 2.0.6.
We use an internal framework that basically wraps spring-boot and several other dependencies that our applications need (like log4j and spring-cloud-gcp-pubsub). This internal framework combined the log4j update with the spring-cloud-gcp-pubsub version 2.0.5 -> 2.0.6; we noticed the issues when trying to upgrade the internal framework to update log4j. We managed to upgrade log4j separately (which is what I was doing yesterday). Now that the urgent issue is resolved, we will investigate further to see if we can in fact pinpoint it to spring-cloud-gcp-pubsub version 2.0.5 -> 2.0.6 and if so provide more information.

[mlvandijk]

Update: The issue with crashing pods might be related to the fact that gcp 2.0.6 has also a new health indicator for spanner. See https://github.com/GoogleCloudPlatform/spring-cloud-gcp/pull/643/files - we are confirming now (but it's the end of the work day here)

[mlvandijk]

We narrowed it down to a Spanner health check that was enabled by default. Disabling it fixed our problems (other than the original issue). Thanks for your help & quick responses!

[meltsufin]

@mlvandijk Can you provide some more information on how the Spanner health checking was causing problems? Are you using Spanner in the application? The health check should not be turned on, unless you're using the Spanner Spring Boot starter.

[elefeint]

Spring Boot Actuator warns against using health checks to determine kubernetes liveness, but we've seen users of Spring Cloud GCP have production issues because Kubernetes liveness was tied to Spring Boot Actuator healthchecks. Is using health checks in liveness indicator some kind of default somewhere?

[mlvandijk]

Yes, we are using Spanner. The health check was turned on by default (that might have been done by our internal framework); we turned it off.

[mlvandijk]

Afaik it was this change: https://github.com/GoogleCloudPlatform/spring-cloud-gcp/pull/643/files
The comment there states:

NOTE: If your application already has actuator and Cloud Spanner starters, this health indicator is enabled by default.
To disable the Cloud Spanner indicator, set `management.health.spanner.enabled` to `false`.

We set management.health.spanner.enabled to false as suggested and that fixed our problems.

[elefeint]

It's correct that most health indicators are enabled by default, but they should not be tied to Liveness group, which is what controls the Kubernetes restarts. So somehow these custom informational indicators are making their way to management.endpoint.health.group.liveness.include.