Make the relationship between the Reporter and Closeable interfaces explicit. #1307
Conversation
…obfischer for the initial implementation.
…n JmxReporter and ScheduledReporter
|
Pushed a fix for the codeclimate issue. There seems to be an unrelated test feailure in metrics-httpasyncclient though, not sure what to do about that. |
| * @see ConsoleReporter | ||
| * @see CsvReporter | ||
| * @see Slf4jReporter | ||
| */ | ||
| public abstract class ScheduledReporter implements Closeable, Reporter { | ||
| public abstract class ScheduledReporter implements Reporter { |
There was a problem hiding this comment.
Please don't remove Closeable from ScheduledReporter, because the change is binary incompatible. That means we will force all 3rd reporters to upgrade to the latest version of metrics-core. I think it should be fine to just add Closeable to Reporter. It should be backward-compatible because ScheduledReporter and JmxReporter implement it already.
| */ | ||
| public class JmxReporter implements Reporter, Closeable { | ||
| public class JmxReporter implements Reporter { |
There was a problem hiding this comment.
The same comment as for ScheduledReporter
…inary compatibility
|
I hadn't considered that removing the interface would cause problems but it makes sense. Updatd the PR to address the issue. Also, the same metrics-httpasyncclient error seems to have occurred again. |
|
Thanks, I will probably ignore |
|
Thanks for the contribution! |
Resolves #1305 and reimplements #743. Credit to @obfischer for the initial implementation.
This makes it easier to denote that any
Reporteris automaticallyCloseablewhen passing around the genericReporterinterface. Making the relationship explicit eases situations where a user may receive either aJmxReporteror some subclass ofScheduledReporter.