Add Kafka Monitor code #2
Conversation
|
|
||
| for (Map.Entry<String, Properties> entry : testProps.entrySet()) { | ||
| String name = entry.getKey(); | ||
| if (name.lastIndexOf('-') != -1) |
There was a problem hiding this comment.
Did we document anywhere about this expected format with "-"?
There was a problem hiding this comment.
Thanks for the review!
It is not documented yet. I need to document the format of the json file provided to KafkaMonitor.
There was a problem hiding this comment.
I had the same question when I first saw this
There was a problem hiding this comment.
@jjkoshy @becketqin It is documented in the config/kafka-monitor.properties now. See https://github.com/lindong28/kafka-monitor/blob/first-commit/config/kafka-monitor.properties. Does this address your concern?
There was a problem hiding this comment.
Sure, but it seems slightly hacky. Is it necessary to add those specific numbers? i.e., shouldn't we just let the user specify as many instances as they want and the instantiation will add specific suffixes if necessary (say to avoid metrics collisions)
There was a problem hiding this comment.
@jjkoshy Here are two reasons for having these specific numbers. First, it allows Kafka Monitor to explicitly provide log information for each test. Say user starts to BasicEndToEnd test with different configs to monitor the same kafka clusters. If one BasicEndToEnd test exits unexpectedly, or assertions fail for only this BasicEndToEnd test, then we want to alert users that one specific test has failed. Having user-specified name will be helpful in this case.
Another reason is that this makes it easy to parse the properties file. KafkaMontor.main() reads the properties file into a Map and we need to keep the name distinct to allow multiple instances of the same test class to be created by Kafka Monitor.
Given these two reasons, do you think this format makes sense?
| import java.util.concurrent.ScheduledExecutorService; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| public class MetricsExportService implements Service { |
There was a problem hiding this comment.
Would prefer calling this say DefaultMetricsReportingService or similar
There was a problem hiding this comment.
Would DefaultMetricsReportService be more concise and consistent with names of ProduceService and ConsumeService?
There was a problem hiding this comment.
How about DefaultMetricsReporterService ? i.e., it is sort of standard wrt metrics Reporter interfaces (such as those with yammer and Kafka metrics)
…ics and logs for different instances of the same service/test. All user to specified report.metrics.filter with wild card.
* Add HA monitoring using Abstract Coordinator * Change some naming * Update log messages * Add service factory for HA monitoring * Update src/main/java/com/linkedin/xinfra/monitor/XinfraMonitor.java Check for HAMonitoring class more robustly. Co-authored-by: hgeraldino <[email protected]> * Correct build error * Cleanup and clarify HA variable Co-authored-by: Christopher Beard <[email protected]> Co-authored-by: hgeraldino <[email protected]>
4 participants
No description provided.