Congrats on the Milestone and a couple of suggestions

[tomazfernandes]

Hey there! Congrats on the first Milestone!

I have a couple of quick suggestions for your consideration.

In these and a couple of other parts you lookup the ContainerProperties when the Container is already running. Unless I'm missing something, ContainerProperties is a mutable object and users can change those settings at will.

https://github.com/spring-projects-experimental/spring-pulsar/blob/10ef7000a9ab6c28768fa3aa53fb6431b06ea23f/spring-pulsar/src/main/java/org/springframework/pulsar/listener/DefaultPulsarMessageListenerContainer.java#L285-L289

https://github.com/spring-projects-experimental/spring-pulsar/blob/10ef7000a9ab6c28768fa3aa53fb6431b06ea23f/spring-pulsar/src/main/java/org/springframework/pulsar/listener/DefaultPulsarMessageListenerContainer.java#L300-L304

Is a user changing these settings with the Container running a feature? Otherwise, isn't it better to maybe extract these properties to final fields in the inner Listener class when the container starts and look that up instead?

Another part I'm not too sure about is this one:

https://github.com/spring-projects-experimental/spring-pulsar/blob/10ef7000a9ab6c28768fa3aa53fb6431b06ea23f/spring-pulsar/src/main/java/org/springframework/pulsar/listener/DefaultPulsarMessageListenerContainer.java#L456-L458

You seem to be using a concurrency structure in a non concurrency-safe way. If this isn't concurrent code, why not use a plain boolean? Or, if it is concurrent code, isn't the compareAndSet method a better choice, instead of using separate get and set calls?

WDYT, makes sense, or maybe I'm missing something? Thanks!

[sobychacko]

@tomazfernandes You are right. We don't expect the users to change the ContainerProperties once the container starts running (at least per the current design). So, we can make them final in the inner class.

+1 for calling compareAndSet.

Any chance you can send a PR cleaning up these? That would be awesome!

[sobychacko]

@tomazfernandes I created this issue to track this discussion: #133

[tomazfernandes]

Hey Soby! Just for clarification, AFAIK users are supposed to change container properties at will, but such changes are supposed to be applied only on container restart - hence attributing the values to final variables when the Listener is created in the container's doStart method.

Here's similar code from Spring Kafka in case you want to take a look.

Unfortunately I've got a lot going on here and don't think I'll be able to write a PR for this. Sorry! Just these two things called my attention and I wanted to share with you and the team.

Hope it's helpful! Thanks!

[sobychacko]

No worries, @tomazfernandes. We will take a closer look. Thank you for pointing them out!

[tomazfernandes]

No problem! I'll keep an eye open for anything else, overall the project looks great! Hopefully I'll have the time to contribute with a PR soon. In the meantime, please let me know if there's anything else I can help with.

Thanks!

[onobc]

Doing a bit of GH discussions housekeeping...

Closing this as resolved.

Thanks @tomazfernandes for the discussion.