Add Pulsar config props for startup policy #42180
Conversation
This commit adds properties to configure the startup behavior for the Pulsar message containers that back the `@PulsarListener`, `@ReactivePulsarListener`, and `@PulsarReader`.
spring-projects-issues
added
the
status: waiting-for-triage
mhalbritter
added
type: enhancement
There was a problem hiding this comment.
Thanks for the PR. I've added some nit and I would have processed them myself but I'd like some confirmation about the default values and, potentially, a code change in Spring Pulsar to avoid code duplication.
Also, can you please build locally before pushing?
| public static class Startup { | ||
|
|
||
| /** | ||
| * The max time to wait for the container to start. |
There was a problem hiding this comment.
Description of configuration properties do not start with the, a, etc. I suggest Time to wait for the container to start.
| private Duration timeout; | ||
|
|
||
| /** | ||
| * The action to take when the container fails to start. |
There was a problem hiding this comment.
Same. I suggest Action to take when the container fails to start.
| /** | ||
| * The max time to wait for the container to start. | ||
| */ | ||
| private Duration timeout; |
There was a problem hiding this comment.
Is there a default for this property? If so I think we should use the opportunity to set the default so that it's documented, and very in a test that it's consistent in case it changes.
| /** | ||
| * The action to take when the container fails to start. | ||
| */ | ||
| private FailurePolicy onFailure; |
There was a problem hiding this comment.
I understand the previous behavior was to continue and log an exception. There's no default value here so it's hard to see if that's still the case. If that's the case, this field should have a default value that matches the default behavior.
| customizeReaderStartupProperties(readerContainerProperties); | ||
| } | ||
|
|
||
| private void customizeReaderStartupProperties(PulsarReaderContainerProperties readerContainerProperties) { |
There was a problem hiding this comment.
It's unfortunate that this is the exact same code as customizeListenerStartupProperties? Since the feature is shared, isn't there a way to avoid the code duplication?
There was a problem hiding this comment.
The code to de-duplicate was more complicated than having this in multiple places.
| customizeListenerStartupProperties(containerProperties); | ||
| } | ||
|
|
||
| private void customizeListenerStartupProperties(ReactivePulsarContainerProperties<?> containerProperties) { |
snicoll
added
the
status: waiting-for-feedback
spring-projects-issues
added
status: feedback-provided
Yes, I saw the red builds. I was building locally (had already run tests w/in IDEA) but was not running checks as I was using This |
|
Closing in favor of #42182 |
wilkinsona
added
the
status: declined
This commit adds properties to configure the startup behavior for the Pulsar message containers that back the
@PulsarListener,@ReactivePulsarListener, and@PulsarReader.This is to support the recently added startup policy feature in Spring for Apache Pulsar.