Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rework PhiAccrualFailureDetector to enable monitoring of interval #1137

Merged
merged 5 commits into from
Feb 26, 2024
Merged

Rework PhiAccrualFailureDetector to enable monitoring of interval #1137

merged 5 commits into from
Feb 26, 2024

Conversation

nvollmar
Copy link
Contributor

resolves #1136

danischroeter
danischroeter approved these changes Feb 21, 2024
View reviewed changes
Copy link

@danischroeter danischroeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@pjfanning
Copy link
Contributor

Would it be possible to write a fuller description to the issue? This is a non-trivial change to our code and we need to be able to release note it.

@nvollmar
Copy link
Contributor Author

nvollmar commented Feb 21, 2024 •
edited
Loading

@pjfanning I can add more to the issue. But I’d consider this a rather trivial change.

There is no functional difference, the implementation is now open for Kanela to intercept the call to the new method to report that metric to Kamon. Or to extend this class and overwrite the method to get the interval.

Also the new trait makes it possible to implement a custom failure detector with the same functionality.
This isn’t really needed for our case, but we find it good design to not match on a specific implementation when that one is replaceable by design.

mdedetrich
mdedetrich approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, as far as I can tell this is just shuffling code around so certain parts of it can be overridden (i.e. enabling features like monitoring interval).

@nvollmar
Copy link
Contributor Author

Looks like link validator is broken because the url changed from opensource.org/licenses/BSD-3-Clause to opensource.org/blog/license/bsd-3-clause

@mdedetrich
Copy link
Contributor

Looks like link validator is broken because the url changed from opensource.org/licenses/BSD-3-Clause to opensource.org/blog/license/bsd-3-clause

This is unrelated to this PR so you can file an issue or fix it in another PR

He-Pin
He-Pin reviewed Feb 22, 2024
View reviewed changes
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the usercase is for aop,then we should better have javadsl in mind too.

@nvollmar
Copy link
Contributor Author

The use case would be to intercept or override the call to recordInterval

@nvollmar nvollmar requested a review from He-Pin February 23, 2024 10:42
He-Pin
He-Pin approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@nvollmar
Copy link
Contributor Author

Had to add a mima exclude for the removed private[pekko] method

@nvollmar
Copy link
Contributor Author

This would be ready to merge unless there are some objections/further remarks

Roiocam
Roiocam approved these changes Feb 26, 2024
View reviewed changes
Copy link
Member

@Roiocam Roiocam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mdedetrich
mdedetrich requested changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor thing, want to add a NOTE prefix to comment so that it appears in IDE"s then I will approve

remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala Outdated Show resolved Hide resolved
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala Outdated Show resolved Hide resolved
mdedetrich
mdedetrich approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@mdedetrich mdedetrich left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just added a commit to capitalize the note

@nvollmar nvollmar merged commit 613a45b into apache:main Feb 26, 2024
18 checks passed
@nvollmar nvollmar deleted the heartbeat-monitoring-wip branch February 26, 2024 11:27
nvollmar added a commit to nvollmar/incubator-pekko that referenced this pull request Feb 26, 2024
@nvollmar
Rework PhiAccrualFailureDetector to enable monitoring of interval (ap…
ff5bfbc 
…ache#1137)

* Introduce trait to avoid matching concrete subclass
* Extract interval method to make it available for recording metrics

---------
Co-authored-by: Matthew de Detrich <[email protected]>
@nvollmar nvollmar mentioned this pull request Feb 26, 2024
Closed
@pjfanning pjfanning added this to the 1.1.0-M1 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roiocam Roiocam Roiocam approved these changes

@danischroeter danischroeter danischroeter approved these changes

@mdedetrich mdedetrich mdedetrich approved these changes

@He-Pin He-Pin He-Pin approved these changes

@pjfanning pjfanning Awaiting requested review from pjfanning

@laglangyue laglangyue Awaiting requested review from laglangyue

@jxnu-liguobin jxnu-liguobin Awaiting requested review from jxnu-liguobin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1.0-M1
Development

Successfully merging this pull request may close these issues.

interceptable heartbeat interval to record metric
8 participants
@nvollmar @pjfanning @mdedetrich @He-Pin @danischroeter @Roiocam @jxnu-liguobin @laglangyue