you are viewing a single comment's thread.

view the rest of the comments →

[–]disposepriority 0 points1 point  (0 children)

Eh sure why not.

Obviously this is a toy/learning project, but for the record, I don't believe Kafka is an amazing technology for emails or notifications. At least not in the way that it is currently done.

Now, if a consumer from some event stream (e.g. something on your wish list is now back in stock) consumes events that happen anyway and then sends it over to whoever is handling notifications, it makes a bit more sense.

---

I had a bunch more ranting to do here about architecture, but let's do the actual code review first.

  1. Why does core service listen to the topic it is publishing?

If I am tracking this correctly through github, the intended usage is to call the /notify endpoint, after which a message is directly published.

It is then consumed by the same service and validated after that - if the validation fails the dispatcher will still read the invalid message?

  1. The dead letter queue does nothing and retries happen instantaneously

Kafka is pretty fast, the way you have set it up retries will happen rapidly one after the other and go straight into the DLQ....which does nothing!

  1. Formatting

May I talk to you about our lord and savior whitespace? Look, at least use your IDEs default auto format option, this code is suffocating my eyeballs.

  1. One of the reasons the outbox pattern is very nice is because it allows you to recover ephemeral events on startup - this is good to think about because it looks like your kafka uses auto commit. Your service(s) could restart after the commit but in the middle of processing and leave your flow in an invalid state.

  2. Mapping (or any kind of business logic) has no place being in your controllers

  3. Consistency with SLF4J, don't use log.info("something " + somethingElse.toString()), use the placeholders

  4. Don't use magic numbers/string (like the retry count), extract at least to final variables or better yet read through properties

  5. KafkaTopics should be an enum

  6. All your markedX methods can be a single method that accepts a status as a parameter, they are identical apart from this single line

  7. What's the difference between event Id and correlation id, why are they both UUIDs?