[jira] [Comment Edited] (CASSANDRA-13457) Diag. Events: Add base classes
[ https://issues.apache.org/jira/browse/CASSANDRA-13457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16392139#comment-16392139 ] mck edited comment on CASSANDRA-13457 at 3/9/18 12:07 AM: -- [~spo...@gmail.com], {quote}`diagnostic_events_enabled: true` … [13b1e8adbd|https://github.com/apache/cassandra/commit/13b1e8adbd3f597311b7f67d71318dc939dd54fb] {quote} +1 {quote}`DiagnosticEvent.addIfNotNull(..)` … [5770638d6|https://github.com/apache/cassandra/commit/5770638d689333d59f57a9ef977dbb1fa6964e30] {quote} +1 {quote}What's the latest trend handling this? {quote} I see more constructor IoC happening in places, the new streaming patches in 4.0 for example. I guess it's mainly about avoiding the static functionality, keeping static down to the `instance` field. `MessagingService.instance()` does it well, in that it also does the lazy-singleton-initialisation approach via the use of the `MSHandle` inner static class. I had a question about whether we can/should go further and separate the concerns (bean vs function) in the events classes, in this [comment|https://github.com/apache/cassandra/commit/ba09523de7cad3b3732c0e7e60b072f84e809e21#r28005488]. {quote}The {{GossiperEvent}} will access these members from the constructor to collect relevant details for the event. {quote} Gotcha, I missed that, thanks. {quote}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. {quote} To clarify, I didn't mean serialising classes. I was only thinking that the event classes could pair the `Map toMap(..)` method with a 'MyEvent fromString(..)` as a convenience to anyone in the java space. But I agree, it's premature and would only cause compatibility headaches. {quote} That's also the reason I'd prefer to keep classes public, so they can be used for unit testing. {quote} So long as the unit test is also in the same package the class can be package-protected. {quote}After some basic testing I quickly realized that way too much events would be produced, if you happen to subscribe to them. {quote} You're quite right. The frequency of calls increases diagnostics, to tracing, to metrics. What's written in that paragraph Stefan would make good dev documentation about when and how to use each throughout the codebase. The point that "would enable/disable repair tracing through the {{diagnostic_events_enabled}} flag" is enough to warrant not worrying about any attempts at pairing events at this point in time. Maybe it's part of a bigger exercise later looking for what existing trace events should also be diagnostic events. was (Author: michaelsembwever): [~spo...@gmail.com], {quote}`diagnostic_events_enabled: true` [13b1e8adbd|https://github.com/apache/cassandra/commit/13b1e8adbd3f597311b7f67d71318dc939dd54fb] {quote} +1 {quote}`DiagnosticEvent.addIfNotNull(..)` [5770638d6|https://github.com/apache/cassandra/commit/5770638d689333d59f57a9ef977dbb1fa6964e30] {quote} +1 {quote}What's the latest trend handling this? {quote} I see more constructor IoC happening in places, the new netty streaming patches in 4.0 for example. I guess it's mainly about avoiding the static functionality, keeping static down to the `instance` field. `MessagingService.instance()` does it well, in that it also does the lazy-singleton-initialisation approach via the use of the `MSHandle` inner static class. I had a question about whether we can/should go further and separate the concerns (bean vs function) in the events classes, in this [comment|[https://github.com/apache/cassandra/commit/ba09523de7cad3b3732c0e7e60b072f84e809e21#r28005488]|https://github.com/apache/cassandra/commit/ba09523de7cad3b3732c0e7e60b072f84e809e21#r28005488].] {quote}The {{GossiperEvent}} will access these members from the constructor to collect relevant details for the event. {quote} Gotcha, I missed that, thanks. {quote}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. {quote} To clarify, I didn't mean serialising classes. I was only thinking that the event classes could pair the `Map toMap(..)` method with a 'MyEvent fromString(..)` as a convenience to anyone in the java space. But I agree, it's premature and would only cause compatibility headaches. {quote} That's also the reason I'd prefer to keep classes public, so they can be used for unit testing. {quote} So long as the unit test is also in the same package the class can be package-protected. {quote}After some basic testing I quickly realized that way too much events would be produced, if you happen to subscribe to them. {quote} You're quite right. The frequency of calls increases diagnostics, to tracing,
[jira] [Comment Edited] (CASSANDRA-13457) Diag. Events: Add base classes
[ https://issues.apache.org/jira/browse/CASSANDRA-13457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16392139#comment-16392139 ] mck edited comment on CASSANDRA-13457 at 3/9/18 12:07 AM: -- [~spo...@gmail.com], {quote}`diagnostic_events_enabled: true` … [13b1e8adbd|https://github.com/apache/cassandra/commit/13b1e8adbd3f597311b7f67d71318dc939dd54fb] {quote} +1 {quote}`DiagnosticEvent.addIfNotNull(..)` … [5770638d6|https://github.com/apache/cassandra/commit/5770638d689333d59f57a9ef977dbb1fa6964e30] {quote} +1 {quote}What's the latest trend handling this? {quote} I see more constructor IoC happening in places, the new streaming patches in 4.0 for example. I guess it's mainly about avoiding the static functionality, keeping static down to the `instance` field. `MessagingService.instance()` does it well, in that it also does the lazy-singleton-initialisation approach via the use of the `MSHandle` inner static class. I had a question about whether we can/should go further and separate the concerns (bean vs function) in the events classes, in this [comment|https://github.com/apache/cassandra/commit/ba09523de7cad3b3732c0e7e60b072f84e809e21#r28005488]. {quote}The {{GossiperEvent}} will access these members from the constructor to collect relevant details for the event. {quote} Gotcha, I missed that, thanks. {quote}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. {quote} To clarify, I didn't mean serialising classes. I was only thinking that the event classes could pair the `Map toMap(..)` method with a 'MyEvent fromString(..)` as a convenience to anyone in the java space. But I agree, it's premature and would only cause compatibility headaches. {quote} That's also the reason I'd prefer to keep classes public, so they can be used for unit testing. {quote} So long as the unit test is also in the same package the class can be package-protected. {quote}After some basic testing I quickly realized that way too much events would be produced, if you happen to subscribe to them. {quote} You're quite right. The frequency of calls increases diagnostics, to tracing, to metrics. What's written in that paragraph Stefan would make good dev documentation about when and how to use each throughout the codebase. The point you raise that "would enable/disable repair tracing through the {{diagnostic_events_enabled}} flag" is enough imo to warrant not worrying about any attempts at pairing events at this point in time. Maybe it's part of a bigger exercise later looking for what existing trace events should also be diagnostic events. was (Author: michaelsembwever): [~spo...@gmail.com], {quote}`diagnostic_events_enabled: true` … [13b1e8adbd|https://github.com/apache/cassandra/commit/13b1e8adbd3f597311b7f67d71318dc939dd54fb] {quote} +1 {quote}`DiagnosticEvent.addIfNotNull(..)` … [5770638d6|https://github.com/apache/cassandra/commit/5770638d689333d59f57a9ef977dbb1fa6964e30] {quote} +1 {quote}What's the latest trend handling this? {quote} I see more constructor IoC happening in places, the new streaming patches in 4.0 for example. I guess it's mainly about avoiding the static functionality, keeping static down to the `instance` field. `MessagingService.instance()` does it well, in that it also does the lazy-singleton-initialisation approach via the use of the `MSHandle` inner static class. I had a question about whether we can/should go further and separate the concerns (bean vs function) in the events classes, in this [comment|https://github.com/apache/cassandra/commit/ba09523de7cad3b3732c0e7e60b072f84e809e21#r28005488]. {quote}The {{GossiperEvent}} will access these members from the constructor to collect relevant details for the event. {quote} Gotcha, I missed that, thanks. {quote}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. {quote} To clarify, I didn't mean serialising classes. I was only thinking that the event classes could pair the `Map toMap(..)` method with a 'MyEvent fromString(..)` as a convenience to anyone in the java space. But I agree, it's premature and would only cause compatibility headaches. {quote} That's also the reason I'd prefer to keep classes public, so they can be used for unit testing. {quote} So long as the unit test is also in the same package the class can be package-protected. {quote}After some basic testing I quickly realized that way too much events would be produced, if you happen to subscribe to them. {quote} You're quite right. The frequency of calls increases diagnostics, to tracing, to metrics. What's written in that paragraph Stefan would make good dev documentation about when and h
[jira] [Comment Edited] (CASSANDRA-13457) Diag. Events: Add base classes
[ https://issues.apache.org/jira/browse/CASSANDRA-13457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16389371#comment-16389371 ] mck edited comment on CASSANDRA-13457 at 3/7/18 11:27 AM: -- hey [~spo...@gmail.com], looks all awesome, so a +1 from me. i do have some small questions/thoughts… {{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? {{DiagnosticEvent.addIfNotNull(..)}} - this seems it could be a bit trivial… Can we just enforce toString serialisation in the subclasses instead? (like Hint does it) {{DiagnosticEventService}} - 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… - 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 - 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) - What about conglomeration between metrics, diag events, and tracing events? For example when would the latter two not ever pair?, good example in HintsDispatcher was (Author: michaelsembwever): hey [~spo...@gmail.com], looks all awesome, so a +1 from me. i do have some small questions/thoughts… {{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? {{DiagnosticEvent.addIfNotNull(..)}} - this seems it could be a bit trivial… Can we just enforce toString serialisation in the subclasses instead? (like Hint does it) {{DiagnosticEventService}} - 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… - 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 - Should the event classes be included in the client jarfile. 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) - What about conglomeration between metrics, diag events, and tracing events? For example when would the latter two not ever pair?, good example in HintsDispatcher > 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
[jira] [Comment Edited] (CASSANDRA-13457) Diag. Events: Add base classes
[ https://issues.apache.org/jira/browse/CASSANDRA-13457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16389371#comment-16389371 ] mck edited comment on CASSANDRA-13457 at 3/7/18 11:27 AM: -- hey [~spo...@gmail.com], looks all awesome, so a +1 from me. i do have some small questions/thoughts… {{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? {{DiagnosticEvent.addIfNotNull(..)}} - this seems it could be a bit trivial… Can we just enforce toString serialisation in the subclasses instead? (like Hint does it) {{DiagnosticEventService}} - 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… - 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 - 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) - 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 was (Author: michaelsembwever): hey [~spo...@gmail.com], looks all awesome, so a +1 from me. i do have some small questions/thoughts… {{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? {{DiagnosticEvent.addIfNotNull(..)}} - this seems it could be a bit trivial… Can we just enforce toString serialisation in the subclasses instead? (like Hint does it) {{DiagnosticEventService}} - 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… - 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 - 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) - What about conglomeration between metrics, diag events, and tracing events? For example when would the latter two not ever pair?, good example in HintsDispatcher > 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