-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Auto-create Pulsar topics #64
Conversation
Thanks, @alpreu. This is a great start. This is precisely something we had in mind as a high-priority feature item. Much appreciated you started with it. Please let us know once it is ready for review. |
1bce6b0
to
6ee3895
Compare
6ee3895
to
045b63b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alpreu awesome work! I like how you applied the nested testconfig. I made some simplification requests on them as well. This is looking good though. Ping me if you have any questions and thank you for this contribution.
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarAdministration.java
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarAdministration.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarAdministration.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarAdministration.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarAdministration.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarAdministration.java
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarTopicBuilder.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/test/java/org/springframework/pulsar/core/PulsarAdministrationTests.java
Outdated
Show resolved
Hide resolved
...ulsar/src/test/java/org/springframework/pulsar/core/PulsarAdministrationBadContextTests.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/test/java/org/springframework/pulsar/core/PulsarTopicTests.java
Show resolved
Hide resolved
@alpreu, I know this PR is WIP, but some general comments. Please add copyrights and author tags to all the classes. Also, add javadocs on public API methods/classes, etc. We should also make the |
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarAdministration.java
Outdated
Show resolved
Hide resolved
...ulsar/src/test/java/org/springframework/pulsar/core/PulsarAdministrationBadContextTests.java
Outdated
Show resolved
Hide resolved
spring-pulsar/src/test/java/org/springframework/pulsar/core/PulsarTopicTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alpreu I added a few minor comments to address but other than that, the code side of this is complete.
I think the final things are:
- Update docs (as you mentioned earlier)
- Consider adding this to one or both sample apps
Thanks
@sobychacko I would be in favor of doing an immediate follow-on PR for the admin auto-configuration. It will require property updates and tests etc.. |
/** | ||
* The Pulsar administration contract. | ||
* | ||
* @author Chris Bono |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep yourself as the single author. We collaborated on it but this is your work my friend. :)
Same goes for all files in here.
Closed via 4bab93e |
WIP PR for #41 , not review ready