Preparing for producer diagnostics #1452
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erikvanoosten
force-pushed
the
producer-diagnostics
branch
2 times, most recently
from
85249b2 to
36bfae6
Compare
|
Hint for the reviewer: enable 'hide whitespace' to minimize the diff. |
It has been a long wish to have producer diagnostics. Although these are not really hard to add, the consumer diagnostics are a bit in the way; these already claim some names and make code sharing awkward. Now that we are moving towards zio-kafka 3.0 we can fix these at the cost of some backward incompatibilities. This PR prepares for the introduction of producer diagnostics by moving the consumer diagnostics specific code out of the diagnostics package, while keeping the sharable code there. The following changes are proposed (fixable with scalafix): | Old | New | | --- | --- | | `zio.kafka.consumer.diagnostics.Diagnostics` | `zio.kafka.consumer.Consumer.ConsumerDiagnostics` | | `zio.kafka.consumer.diagnostics.Diagnostics.NoOp` | `zio.kafka.consumer.Consumer.NoDiagnostics` | | `zio.kafka.consumer.diagnostics.DiagnosticEvent` | `zio.kafka.consumer.ConsumerDiagnosticEvent` | | `zio.kafka.consumer.diagnostics.DiagnosticEvent.*` | `zio.kafka.consumer.ConsumerDiagnosticEvent.*` | | `zio.kafka.consumer.diagnostics.DiagnosticEvent.Finalization.*` | `zio.kafka.consumer.ConsumerDiagnosticEvent.*` | Different invocation: old: `zio.kafka.consumer.diagnostics.Diagnostics.SligingQueue.make()` new: `zio.kafka.diagnostics.SlidingDiagnostics.make[ConsumerDiagnosticEvent]()` Move to internal or test package: | Old | New | New location | | --- | --- | --- | | `zio.kafka.consumer.diagnostics.Diagnostics.ConcurrentDiagnostics` | `zio.kafka.diagnostics.internal.ConcurrentDiagnostics` | internal | | `zio.kafka.consumer.diagnostics.DiagnosticEvent.Finalization` | removed | - | Notes: - The intermediate trait `DiagnosticEvent.Finalization` was removed from the type hierarchy because it doesn't serve any user purpose (it was introduced to make it easier to test the shutdown sequence of the consumer).
erikvanoosten
force-pushed
the
producer-diagnostics
branch
from
36bfae6 to
636aab0
Compare
vermas7988
reviewed
Mar 1, 2025
vermas7988
reviewed
Mar 1, 2025
zio-kafka/src/main/scala/zio/kafka/diagnostics/internal/ConcurrentDiagnostics.scala
Show resolved
Hide resolved
|
I can see the use in generalizing SlidingDiagnostics and ConcurrentDiagnostics by parameterizing them with the type of diagnostic event. But I'd like to see if we can keep the rest of the stuff in the same package and with the same name, just to minimize changes. What would be the downside of having |
One downside is that its very annoying when you have a scala file that needs both classes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It has been a long wish to have producer diagnostics. Although these are not really hard to add, the consumer diagnostics are a bit in the way; these already claim some names and make code sharing awkward. Now that we are moving towards zio-kafka 3.0 we can fix these at the cost of some backward incompatibilities.
This PR prepares for the introduction of producer diagnostics by moving the consumer diagnostics specific code out of the diagnostics package, while keeping the sharable code there.
The following changes are proposed (fixable with scalafix):
zio.kafka.consumer.diagnostics.Diagnosticszio.kafka.consumer.Consumer.ConsumerDiagnosticszio.kafka.consumer.diagnostics.Diagnostics.NoOpzio.kafka.consumer.Consumer.NoDiagnosticszio.kafka.consumer.diagnostics.DiagnosticEventzio.kafka.consumer.ConsumerDiagnosticEventzio.kafka.consumer.diagnostics.DiagnosticEvent.*zio.kafka.consumer.ConsumerDiagnosticEvent.*zio.kafka.consumer.diagnostics.DiagnosticEvent.Finalization.*zio.kafka.consumer.ConsumerDiagnosticEvent.*Different invocation:
old:
zio.kafka.consumer.diagnostics.Diagnostics.SligingQueue.make()new:
zio.kafka.diagnostics.SlidingDiagnostics.make[ConsumerDiagnosticEvent]()Move to internal or test package:
zio.kafka.consumer.diagnostics.Diagnostics.ConcurrentDiagnosticszio.kafka.diagnostics.internal.ConcurrentDiagnosticszio.kafka.consumer.diagnostics.DiagnosticEvent.FinalizationNotes:
DiagnosticEvent.Finalizationwas removed from the type hierarchy because it doesn't serve any user purpose (it was introduced to make it easier to test the shutdown sequence of the consumer).