[
https://issues.apache.org/jira/browse/CASSANDRA-13457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16572448#comment-16572448
]
Jason Brown commented on CASSANDRA-13457:
-----------------------------------------
I was able to scrape a little bit of time to give this patch a semi-decent
read, but this is not a full, formal review. I'm leaving that to
[~michaelsembwever] and any others, but I have some comments/questions below.
First, though, to [~spo...@gmail.com]'s question: I'm largely fine with these
changes wrt runtime impact when the diagnostic events are disabled.
- Config.diagnostic_events_enabled needs to be volatile if you will modify it
at runtime via JMX.
- nit: TokenAllocatorEvent's ctor has a huge parameter list. Consider adding a
Builder as it's the pattern we are gnerally moving to, although I'm not sure
that's actually documented or communicated anywhere. HintEvent and SchemaEvent,
as well.
- DiagnosticEventService.publish() is invoked on the same thread as the caller.
What happens if one of the consumers slows down (because it relies on some kind
of blocking IO, disk, network, whatever). In the case of gossip that have
fairly bad consequences.
- Please document why the map's value type of the return from
DiagnosticEvent.toMap() is Serializable. (I looked at CASSANDRA-14435, and it's
to support transferring the maps via JMX). However, before I saw that ticket I
was quite surprised as to why Serializable as I usually assume "java object
serialization".
- I'm not sure what to think about DiagnosticEvent.getSource(). It's an Object,
which is rather non-descript, type-wise. What is the intention here? Do some of
the follow up tickets use this method? I assume they need to know
implementation details of the specific DiagnosticEvent implementation in order
to figure out qhat the type od the source is.
- Gossiper - I would rather have callers ask for immutable copies, via getter
methods, of essential gossip data structures, rather than change the visibility
of those structures. Directly shared those mutatble data structures makes me
very nervous.
- You've added toString() implementations to about 10 classes. Are those for
debugging help? If so, ignore the rest of this bullet point. If it's for
diagnostic events stuff, I'm worried that the use of toString() now ties any
debugging information or formatting we might want to add is now is bound to any
diagnostic consumers consumers - meaning, we couldn't be sure that in modifying
a class's {{toString()}} we wouldn't be breaking some consumer's logic.
- DiagnosticEvent uses millis for for the timestamp, and in particular,
System.currentTimeMillis(). That method is not guaranteed to be monotonic (nor
are wall clocks). I generally advise against using wall-clock timestamps, but
what are the requirements here?
> 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
