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 2021/06/24 13:27:00 UTC
[GitHub] [ignite] NSAmelchev opened a new pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
NSAmelchev opened a new pull request #9195:
URL: https://github.com/apache/ignite/pull/9195
Remove data region/cache/cache group metrics on deactivation without removing metrics configuration.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] ololo3000 commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r659357057
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -136,6 +136,44 @@ public void testCacheMetricsConfigurationNotRemovedOnStop() throws Exception {
checkMetricsEmpty(cachePrefix);
}
+ /** */
+ @Test
+ public void testCacheMetricsConfigurationNotRemovedOnStopTwoCachesPerGroup() throws Exception {
+ String cachePrefix = cacheMetricsRegistryName("other-cache", false);
Review comment:
Should we use nearEnabled flag instead of `false` here?
--
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] NSAmelchev commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r659563346
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -204,12 +253,14 @@ private void checkMetricsEmpty(String cachePrefix) {
Review comment:
I think this is not related to the issue as well as other refactoring.
--
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] NSAmelchev commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r660318248
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/DataRegionMetricsImpl.java
##########
@@ -726,6 +726,11 @@ public void clear() {
metrics.reset();
}
+ /** Removes all metric for data region. */
+ public void remove() {
+ kernalCtx.metric().remove(metricRegistry().name(), false);
Review comment:
I think code should not be refactored in this issue.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -152,12 +200,25 @@ private void createCache(@Nullable String dataRegionName, @Nullable String cache
if (cacheName != null)
ccfg.setName(cacheName);
+ if (groupName != null)
+ ccfg.setGroupName(groupName);
+
if (nearEnabled)
ccfg.setNearConfiguration(new NearCacheConfiguration());
grid("client").createCache(ccfg);
awaitPartitionMapExchange();
+
Review comment:
No, we can't. We should configure cache and near cache metrics.
--
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 #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
nizhikov merged pull request #9195:
URL: https://github.com/apache/ignite/pull/9195
--
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] ololo3000 commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r659357032
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -204,12 +253,14 @@ private void checkMetricsEmpty(String cachePrefix) {
Review comment:
`MetricRegistry mreg = mmgr.registry(cachePrefix);`
Can we just iterate over mmgr and check that there is no registry with specified name? Mentioned above call creates registry if it does not exist. I see no need for this.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -204,12 +253,14 @@ private void checkMetricsEmpty(String cachePrefix) {
assertNull(mreg.findMetric(metricName(cachePrefix, CACHE_GETS)));
assertNull(mreg.findMetric(metricName(cachePrefix, CACHE_PUTS)));
+ assertNull(mreg.findMetric(metricName(cachePrefix, GET_TIME)));
Review comment:
Here and above, we must use mreg.findMetric(<metric_name>) without registry prefix. Otherwise, this check makes no sense and always true.
--
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] ololo3000 commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r659357077
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -161,6 +200,9 @@ private void createCache(@Nullable String dataRegionName, @Nullable String cache
if (cacheName != null)
ccfg.setName(cacheName);
+ if (groupName != null)
+ ccfg.setGroupName(groupName);
+
Review comment:
I propose to set cache mode as well. It changes in parameters but does not actually apply to the cache.
--
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] ololo3000 commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r659366271
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/DataRegionMetricsImpl.java
##########
@@ -726,6 +726,11 @@ public void clear() {
metrics.reset();
}
+ /** Removes all metric for data region. */
+ public void remove() {
+ kernalCtx.metric().remove(metricRegistry().name(), false);
Review comment:
Can we create dedicated variable for the registry name and calculate it once?
--
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] NSAmelchev commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r660318248
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/DataRegionMetricsImpl.java
##########
@@ -726,6 +726,11 @@ public void clear() {
metrics.reset();
}
+ /** Removes all metric for data region. */
+ public void remove() {
+ kernalCtx.metric().remove(metricRegistry().name(), false);
Review comment:
I think code should not be refactored in this issue.
--
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] NSAmelchev commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r659560806
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -161,6 +200,9 @@ private void createCache(@Nullable String dataRegionName, @Nullable String cache
if (cacheName != null)
ccfg.setName(cacheName);
+ if (groupName != null)
+ ccfg.setGroupName(groupName);
+
Review comment:
I think this class should be refactored. This is not related to the issue and should be done in a separate issue.
--
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] ololo3000 commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r659559825
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -152,12 +200,25 @@ private void createCache(@Nullable String dataRegionName, @Nullable String cache
if (cacheName != null)
ccfg.setName(cacheName);
+ if (groupName != null)
+ ccfg.setGroupName(groupName);
+
if (nearEnabled)
ccfg.setNearConfiguration(new NearCacheConfiguration());
grid("client").createCache(ccfg);
awaitPartitionMapExchange();
+
Review comment:
Can we just pass nearEnabled into cacheMetricsRegistryName instead of if statement?
--
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] NSAmelchev commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r659562362
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -136,6 +136,44 @@ public void testCacheMetricsConfigurationNotRemovedOnStop() throws Exception {
checkMetricsEmpty(cachePrefix);
}
+ /** */
+ @Test
+ public void testCacheMetricsConfigurationNotRemovedOnStopTwoCachesPerGroup() throws Exception {
+ String cachePrefix = cacheMetricsRegistryName("other-cache", false);
Review comment:
Then we should fix `checkMetrics*` methods and other tests to provide the correct cache prefix. I think this is not related to the issue and should be done in a separate issue.
--
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] NSAmelchev commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r660319867
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -152,12 +200,25 @@ private void createCache(@Nullable String dataRegionName, @Nullable String cache
if (cacheName != null)
ccfg.setName(cacheName);
+ if (groupName != null)
+ ccfg.setGroupName(groupName);
+
if (nearEnabled)
ccfg.setNearConfiguration(new NearCacheConfiguration());
grid("client").createCache(ccfg);
awaitPartitionMapExchange();
+
Review comment:
No, we can't. We should configure cache and near cache metrics.
--
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] NSAmelchev commented on a change in pull request #9195: IGNITE-14990 Incorrect values of cache, cache group, data region metrics after cluster re-activation
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #9195:
URL: https://github.com/apache/ignite/pull/9195#discussion_r659564862
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/metric/CacheMetricsAddRemoveTest.java
##########
@@ -204,12 +253,14 @@ private void checkMetricsEmpty(String cachePrefix) {
assertNull(mreg.findMetric(metricName(cachePrefix, CACHE_GETS)));
assertNull(mreg.findMetric(metricName(cachePrefix, CACHE_PUTS)));
+ assertNull(mreg.findMetric(metricName(cachePrefix, GET_TIME)));
Review comment:
Thanks, fixed.
--
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