-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
Add SmallRye Metrics extension to Infinispan Embedded #7935
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @jmartisk
@@ -30,7 +30,6 @@ | |||
<groupId>io.smallrye</groupId> | |||
<artifactId>smallrye-metrics</artifactId> | |||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should remove that one and use quarkus-smallrye-metrics
here and quarkus-smallrye-metrics-deployment
in the deployment module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I have no clue about this metrics stuff :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no deployment-time dependency on metrics in this case. Metrics are handled by Infinispan directly, not by any Quarkus extension, so it all happens in runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I must admit I am not sure what you guys want me to do :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmartisk we might need some reflection registration at some point and so on.
So if you have the runtime dependency, you need the deployment one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed an additional commit with the change.
@jmartisk can you check it's OK with you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsmet sorry I could have done that - I was more asking if I needed to actually do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK, I misunderstood. Let's have @jmartisk check this. I think it's the right thing to do to be on the safe side and be sure SmallRye Metrics behave correctly in native mode. Even if the deployment extension doesn't have anything special for native atm (I haven't checked), it might not always be the case so better be on the safe side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Related to #7910 and #7921