[GitHub] [geode] kirklund opened pull request #3429: GEODE-6609: Protect from MetricsPublishingService exceptions

["kirklund (GitHub)" <gi...@apache.org>]

Catch and log Errors and RuntimeExceptions thrown from instantiation
of or calls to MetricsPublishingService implementations.

Co-authored-by: Michael Oleske <mo...@pivotal.io>

Please review: @demery-pivotal @mhansonp 

[ Full content available at: https://github.com/apache/geode/pull/3429 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] demery-pivotal commented on pull request #3429: GEODE-6609: Protect from MetricsPublishingService exceptions

["demery-pivotal (GitHub)" <gi...@apache.org>]

We also need to protect the call to `addSubregistry()`. I'm not sure about `removeSubregistry()` (I don't know if that ever throws).

[ Full content available at: https://github.com/apache/geode/pull/3429 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #3429: GEODE-6609: Protect from MetricsPublishingService exceptions

["kirklund (GitHub)" <gi...@apache.org>]

The User would be invoking `MetricsSession.addRegistry` from `MetricsPublishingService.start(MetricsSession)`. For example, if the User tries to add a `CompositeMeterRegistry` then `CompositeMeterRegistry.add(MeterRegistry)` would throw IllegalArgumentException which our try-catch in `CacheLifecycleMetricsSession.cacheCreated(InternalCache)` would already be catching and logging.

[ Full content available at: https://github.com/apache/geode/pull/3429 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #3429: GEODE-6609: Protect from MetricsPublishingService exceptions

["kirklund (GitHub)" <gi...@apache.org>]

I think the user would have to use their own thread later on to invoke addSubregistry. Throwing to them is probably good enough. If we catch and log it in calls to addSubregistry, then I think we would want to rethrow so they get a Throwable.

[ Full content available at: https://github.com/apache/geode/pull/3429 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #3429: GEODE-6609: Protect from MetricsPublishingService exceptions

["kirklund (GitHub)" <gi...@apache.org>]

Fixed

[ Full content available at: https://github.com/apache/geode/pull/3429 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] demery-pivotal commented on pull request #3429: GEODE-6609: Protect from MetricsPublishingService exceptions

["demery-pivotal (GitHub)" <gi...@apache.org>]

On `start(session)`, the service might simply stash the session, and defer adding the subregistry until a later time. In that situation, the `addSubregistry()` call can happen outside the safe context of `start(session)`. The risk is low… Is it low enough?

[ Full content available at: https://github.com/apache/geode/pull/3429 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] demery-pivotal commented on pull request #3429: GEODE-6609: Protect from MetricsPublishingService exceptions

["demery-pivotal (GitHub)" <gi...@apache.org>]

This logs "Error invoking start," even though we invoked `stop()`. Perhaps pass the method name to `logError()`?

[ Full content available at: https://github.com/apache/geode/pull/3429 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund closed pull request #3429: GEODE-6609: Protect from MetricsPublishingService exceptions

["kirklund (GitHub)" <gi...@apache.org>]

[ pull request closed by kirklund ]

[ Full content available at: https://github.com/apache/geode/pull/3429 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org