[
https://issues.apache.org/jira/browse/CASSANDRA-13457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16391026#comment-16391026
]
Stefan Podkowinski commented on CASSANDRA-13457:
------------------------------------------------
{quote}diagnostic_events_enabled: true
After recent discussions on the dev ML around the use of experimental flags, eg
on MV, would it make more sense that this was false by default?
{quote}
Publishing events without any subscribers should be noop anyways. I intended
the flag rather as a kill switch, in case the feature is causing any issues in
production. But you're right, it's a new feature and it should probably not be
enabled by default. It's not a big deal to change it to true if you're going to
use it.
([13b1e8adbd|https://github.com/apache/cassandra/commit/13b1e8adbd3f597311b7f67d71318dc939dd54fb])
{quote}DiagnosticEvent.addIfNotNull(..)
this seems it could be a bit trivial… Can we just enforce toString
serialisation in the subclasses instead? (like Hint does it)
{quote}
I'm not really a fan of such helper methods either. I guess my intention was to
keep null values absent from the return mapped and also prevent NPEs. But we
should catch any exception from {{toMap}} anyways, so I'm fine removing it in
favor of {{String.valueOf}} on the caller side.
([5770638d6|https://github.com/apache/cassandra/commit/5770638d689333d59f57a9ef977dbb1fa6964e30])
{quote}Can we avoid the static fields? So to be avoiding adding to the
CASSANDRA-7837 problems… I don't think C* has a better habit in place for this?
But a singleton would be one better than all static fields…
{quote}
I don't mind as long as we try to keep things consistent. What's the latest
trend handling this? {{instance()}}?
But I don't think swapping out the implementation like in {{Trace}} is a use
cases that we have to support here. Basically the "service" is just a
broadcaster that implements publish/subscribe semantics and I can't really
think of any reason someone wants to change that and use their own
implementation.
([ba09523d|https://github.com/apache/cassandra/commit/ba09523de7cad3b3732c0e7e60b072f84e809e21])
{quote}Not too sure why a number of fields in existing classes were changed
from private to package-protected, for example in Gossiper. If it's for tests
in latter branches should they deserve the @VisibleForTesting annotation? And
should that change also happen in the latter branches
{quote}
The {{GossiperEvent}} will access these members from the constructor to collect
relevant details for the event. Passing these values directly from {{Gossiper}}
to {{GossiperEvent.*}} would mean that {{Gossiper}} needs to generate some
garbage by creating immutable copies of some of the values, while at the same
time events might not even be enabled and would be discarded anyways.
{quote}Should the event classes be included in the client jarfile (or some
'type and deserialisation' part of the event classes). This would then
introduce issues of compatibility, (eg enums). If event classes are not exposed
client-side, could they then be package private? (they're not used outside
their package)
{quote}
The implementation in CASSANDRA-13459 would only expose events serialized as
strings. I'd not serialize any internal classes, as we don't have any
versioning in place and classes could change quickly enough to cause
deserialization issues, even on bugfix releases. The only reason for keeping
complex objects as event members would be to make them accessible in unit
tests, see CASSANDRA-13458. That's also the reason I'd prefer to keep classes
public, so they can be used for unit testing.
{quote}What about conglomeration between metrics, diag events, and tracing
events? For example i wonder if the latter two will often pair?, good example
in HintsDispatcher
{quote}
There's probably some overlap, but I'd expect to rather see metrics on hot
paths where you want histograms and need to do local buffering and sampling to
cope with the load. Diag. events should be emitted key events only. As you
mentioned the HintsDispatcher, one of the first versions had events for
dispatched hints. After some basic testing I quickly realized that way too much
events would be produced, if you happen to subscribe to them. You could easily
maintain a histogram for that, but not emit diag events. So it makes sense to
limit emitted events for the hints service to dispatcher and maybe pages.
Although there's some overlap when it comes to looking at the outcome, yes. But
other diag events such as dispatcherCreated, abortRequested, would not make
much sense to have as a metric, as you need a bit more context to make them
useful for further analyzis and the HintEvent has that (hostId, address,
dispatcher,..).
As for tracing events, I'd expect only some pairing between them and diag
events for repairs at this point. Avoiding redundancy should be possible, e.g.
by replacing {{Tracing.traceRepair()}} calls with
{{DiagnosticEventsService.publish()}} calls and by creating a consumer that
would subscribe to all repair events, creates tracing messages and calls
{{Tracing.traceRepair()}} afterwards. But that would mean that we would
enable/disable repair tracing through the {{diagnostic_events_enabled}} flag.
We should maybe do this the other way around then and keep
{{Tracing.traceRepair()}} which in turn would call
{{DiagnosticEventsService.publish()}}.
> Diag. Events: Add base classes
> ------------------------------
>
> Key: CASSANDRA-13457
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13457
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core, Observability
> Reporter: Stefan Podkowinski
> Assignee: Stefan Podkowinski
> Priority: Major
>
> Base ticket for adding classes that will allow you to implement and subscribe
> to events.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
