You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2023/01/17 18:42:47 UTC
[GitHub] [ignite] Vladsz83 opened a new pull request, #10485: IGNITE-18232 : [IEP-80] Remove DataRegionMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Vladsz83 opened a new pull request, #10485:
URL: https://github.com/apache/ignite/pull/10485
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov merged pull request #10485: IGNITE-18232 Remove of CacheMetricsMXBean legacy JMX bean
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov merged PR #10485:
URL: https://github.com/apache/ignite/pull/10485
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by GitBox <gi...@apache.org>.
nizhikov commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1081234368
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheClusterMetricsMXBeanImpl.java:
##########
@@ -17,515 +17,121 @@
package org.apache.ignite.internal.processors.cache;
-import java.util.Collections;
+import javax.cache.management.CacheMXBean;
+import javax.cache.management.CacheStatisticsMXBean;
import org.apache.ignite.IgniteCache;
-import org.apache.ignite.mxbean.CacheMetricsMXBean;
/**
- * Management bean that provides access to {@link IgniteCache IgniteCache}.
+ * MX bean that keeps support of JCache specification.
*/
-public class CacheClusterMetricsMXBeanImpl implements CacheMetricsMXBean {
+public class CacheClusterMetricsMXBeanImpl implements CacheStatisticsMXBean, CacheMXBean {
Review Comment:
It this bean still provide cluster-wide values?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Vladsz83 commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1083841496
##########
modules/core/src/main/java/org/apache/ignite/IgniteCache.java:
##########
@@ -1617,20 +1616,6 @@ public <T> IgniteFuture<Map<K, EntryProcessorResult<T>>> invokeAllAsync(Set<? ex
*/
public CacheMetrics localMetrics();
- /**
- * Gets whole cluster MxBean for this cache.
- *
- * @return MxBean.
- */
- public CacheMetricsMXBean mxBean();
Review Comment:
JCache requires registration of these mx bean by calling CacheManager#enableStatistics(), CacheManager#enableManagement() for example. It doesn't require Cache#mxBean(). Otherwise, we'd have to override the method and would'nt pass the jcache-tck tests. We remove statistics mx beans here. I suggest to hide any related interfaces and calls. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1083956343
##########
modules/clients/src/test/java/org/apache/ignite/internal/processors/rest/RestMemcacheProtocolSelfTest.java:
##########
@@ -185,8 +185,8 @@ public void testReplace() throws Exception {
*/
@Test
public void testMetrics() throws Exception {
- grid().cache(DEFAULT_CACHE_NAME).localMxBean().clear();
- grid().cache(CACHE_NAME).localMxBean().clear();
+ grid().cache(DEFAULT_CACHE_NAME).clearStatistics();
Review Comment:
> You always could clear cache statistics by calling Ignite cache API
What API, exactly? Why in tests you are using private API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Vladsz83 commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1083835143
##########
modules/clients/src/test/java/org/apache/ignite/internal/processors/rest/RestMemcacheProtocolSelfTest.java:
##########
@@ -185,8 +185,8 @@ public void testReplace() throws Exception {
*/
@Test
public void testMetrics() throws Exception {
- grid().cache(DEFAULT_CACHE_NAME).localMxBean().clear();
- grid().cache(CACHE_NAME).localMxBean().clear();
+ grid().cache(DEFAULT_CACHE_NAME).clearStatistics();
Review Comment:
> Why do we need new method `clearStatistics`? AFAICU clear method comes from `javax.cache.management.CacheStatisticsMXBean`
This method calls GridCacheProcessor#clearStatistics(). It isn't new. I brought other call in the tests to keep the conception of mxbean removal.
We keep them now only to support JCache spec. I suggest not to use JCache mxbean interfaces any more even in the tests. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Vladsz83 commented on pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by GitBox <gi...@apache.org>.
Vladsz83 commented on PR #10485:
URL: https://github.com/apache/ignite/pull/10485#issuecomment-1396678879
> It seems like new classes in general just a copy of existing.
JCache's interfaces are smaller, contain fewer methods. JCache's `CacheStatisticsMXBean` has 11 statistics methods. It doesn't have Ignite's `getEntryProcessorHits` or `getRebalancingKeysRate`. And many others from `CacheMetrics`. To support JCache, we don't need entire old CacheClusterMetricsMXBeanImpl. Also, regarding to JCache spec, CacheMXBean and CacheStatisticsMXBean are separated beans. And are enabled/disabled separately by CacheManager#enableManagement() and CacheManager#enableStatistics(). Now we have 2 implementations of these beans not exposed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by GitBox <gi...@apache.org>.
nizhikov commented on PR #10485:
URL: https://github.com/apache/ignite/pull/10485#issuecomment-1396681031
Yes. We should cleanup existing MXBeans and keep only methods required by JCache specification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Vladsz83 commented on pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by GitBox <gi...@apache.org>.
Vladsz83 commented on PR #10485:
URL: https://github.com/apache/ignite/pull/10485#issuecomment-1396688641
> Yes. We should cleanup existing MXBeans and keep only methods required by JCache specification.
It is done in the new beans
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1083760009
##########
modules/core/src/main/java/org/apache/ignite/IgniteCache.java:
##########
@@ -1617,20 +1616,6 @@ public <T> IgniteFuture<Map<K, EntryProcessorResult<T>>> invokeAllAsync(Set<? ex
*/
public CacheMetrics localMetrics();
- /**
- * Gets whole cluster MxBean for this cache.
- *
- * @return MxBean.
- */
- public CacheMetricsMXBean mxBean();
Review Comment:
It seems we can keep this method, but makes it to return `CacheStatisticsMXBean` as required by JCache specification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Vladsz83 commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1083943685
##########
modules/clients/src/test/java/org/apache/ignite/internal/processors/rest/RestMemcacheProtocolSelfTest.java:
##########
@@ -185,8 +185,8 @@ public void testReplace() throws Exception {
*/
@Test
public void testMetrics() throws Exception {
- grid().cache(DEFAULT_CACHE_NAME).localMxBean().clear();
- grid().cache(CACHE_NAME).localMxBean().clear();
+ grid().cache(DEFAULT_CACHE_NAME).clearStatistics();
Review Comment:
> > This method calls GridCacheProcessor#clearStatistics().
>
> How to do the same with public API?
You always could clear cache statistics by calling Ignite cache API and by calling mx bean. Regarding to the IEP, we assume mx bean api pbsolete. Why using It again? Consider we wouldn't have to keep mxbean to support any spec like JCache spec. There would be no mxbean and no old mxbean#clear() method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Vladsz83 commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1083958072
##########
modules/clients/src/test/java/org/apache/ignite/internal/processors/rest/RestMemcacheProtocolSelfTest.java:
##########
@@ -185,8 +185,8 @@ public void testReplace() throws Exception {
*/
@Test
public void testMetrics() throws Exception {
- grid().cache(DEFAULT_CACHE_NAME).localMxBean().clear();
- grid().cache(CACHE_NAME).localMxBean().clear();
+ grid().cache(DEFAULT_CACHE_NAME).clearStatistics();
Review Comment:
> > You always could clear cache statistics by calling Ignite cache API
>
> What API, exactly? Why in tests you are using private API?
It is not private. It uses IgniteCache#clearStatistics(). It is a public API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by GitBox <gi...@apache.org>.
nizhikov commented on PR #10485:
URL: https://github.com/apache/ignite/pull/10485#issuecomment-1396693426
We don't need to copy code from existing to those new classes. Please, revert removal of existing JMX class and keep required methods in it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Vladsz83 commented on pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by GitBox <gi...@apache.org>.
Vladsz83 commented on PR #10485:
URL: https://github.com/apache/ignite/pull/10485#issuecomment-1396907110
> We don't need to copy code from existing to those new classes. Please, revert removal of existing JMX class and keep required methods in it.
Done. I keep CacheClusterMetricsMXBeanImpl. But I think we violate S principle of SOLID. CacheStatisticsMXBean and CacheMXBean should be split and created separately on demand. The old beans implements the both interfaces. However, we pass JCache spec tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by GitBox <gi...@apache.org>.
nizhikov commented on PR #10485:
URL: https://github.com/apache/ignite/pull/10485#issuecomment-1396660024
Can we keep one of `CacheClusterMetricsMXBeanImpl` or `CacheLocalMetricsMXBeanImpl` and implement in it JCache interfaces to reduce changes? It seems like new classes in general just a copy of existing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Vladsz83 commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1083943685
##########
modules/clients/src/test/java/org/apache/ignite/internal/processors/rest/RestMemcacheProtocolSelfTest.java:
##########
@@ -185,8 +185,8 @@ public void testReplace() throws Exception {
*/
@Test
public void testMetrics() throws Exception {
- grid().cache(DEFAULT_CACHE_NAME).localMxBean().clear();
- grid().cache(CACHE_NAME).localMxBean().clear();
+ grid().cache(DEFAULT_CACHE_NAME).clearStatistics();
Review Comment:
> > This method calls GridCacheProcessor#clearStatistics().
>
> How to do the same with public API?
You always could clear cache statistics by calling Ignite cache API and by calling mx bean. Regarding to the IEP, we assume mx bean API is obsolete. Why using it again? Consider we wouldn't have to keep a mxbean to support any spec like JCache spec. There would be no mxbean alt all and no old mxbean#clear() method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Vladsz83 commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by GitBox <gi...@apache.org>.
Vladsz83 commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1081238283
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheClusterMetricsMXBeanImpl.java:
##########
@@ -17,515 +17,121 @@
package org.apache.ignite.internal.processors.cache;
-import java.util.Collections;
+import javax.cache.management.CacheMXBean;
+import javax.cache.management.CacheStatisticsMXBean;
import org.apache.ignite.IgniteCache;
-import org.apache.ignite.mxbean.CacheMetricsMXBean;
/**
- * Management bean that provides access to {@link IgniteCache IgniteCache}.
+ * MX bean that keeps support of JCache specification.
*/
-public class CacheClusterMetricsMXBeanImpl implements CacheMetricsMXBean {
+public class CacheClusterMetricsMXBeanImpl implements CacheStatisticsMXBean, CacheMXBean {
Review Comment:
Yes. It uses `IgniteCache#metrics()`. There is also `IgniteCache#localMetrics()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1083757319
##########
modules/clients/src/test/java/org/apache/ignite/internal/processors/rest/RestMemcacheProtocolSelfTest.java:
##########
@@ -185,8 +185,8 @@ public void testReplace() throws Exception {
*/
@Test
public void testMetrics() throws Exception {
- grid().cache(DEFAULT_CACHE_NAME).localMxBean().clear();
- grid().cache(CACHE_NAME).localMxBean().clear();
+ grid().cache(DEFAULT_CACHE_NAME).clearStatistics();
Review Comment:
Why do we need new method `clearStatistics`?
AFAICU clear method comes from `javax.cache.management.CacheStatisticsMXBean`
Can we just invoke method from MX bean instance as we do now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a diff in pull request #10485: IGNITE-18232 : [IEP-80] Remove of CacheMetricsMXBean legacy JMX bean, v3 : keep JCache beans
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on code in PR #10485:
URL: https://github.com/apache/ignite/pull/10485#discussion_r1083923917
##########
modules/clients/src/test/java/org/apache/ignite/internal/processors/rest/RestMemcacheProtocolSelfTest.java:
##########
@@ -185,8 +185,8 @@ public void testReplace() throws Exception {
*/
@Test
public void testMetrics() throws Exception {
- grid().cache(DEFAULT_CACHE_NAME).localMxBean().clear();
- grid().cache(CACHE_NAME).localMxBean().clear();
+ grid().cache(DEFAULT_CACHE_NAME).clearStatistics();
Review Comment:
> This method calls GridCacheProcessor#clearStatistics().
How to do the same with public API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org