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 2020/02/15 20:25:44 UTC
[GitHub] [ignite] gurustron opened a new pull request #7434: IGNITE-7276
.NET: Enable/disable cache statistics in runtime
gurustron opened a new pull request #7434: IGNITE-7276 .NET: Enable/disable cache statistics in runtime
URL: https://github.com/apache/ignite/pull/7434
IGNITE-7276 .NET: Enable/disable cache statistics in runtime
ICache.EnableStatistics(bool enabled);
IClusterGroup.EnableStatistics(IEnumerable<string> cacheNames, bool enabled)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7434:
IGNITE-7276 .NET: Enable/disable cache statistics in runtime
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7434: IGNITE-7276 .NET: Enable/disable cache statistics in runtime
URL: https://github.com/apache/ignite/pull/7434#discussion_r380323001
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core.Tests/Cache/CacheMetricsTest.cs
##########
@@ -99,6 +99,28 @@ public void TestGlobalMetrics()
Assert.AreEqual(1, remoteMetrics.CachePuts);
}
+ /// <summary>
+ /// Tests the cache metrics enable/disable
+ /// </summary>
+ [Test]
+ public void TestCacheEnableStatistics()
+ {
+ TestEnableStatistics("cacheEnableStatistics", (cache, b) => cache.EnableStatistics(b));
+ }
+
+
+ /// <summary>
+ /// Tests the cache metrics enable/disable
+ /// </summary>
+ [Test]
+ public void TestClusterGroupEnableStatistics()
+ {
+ var cacheName = "clusterEnableStatistics";
+ TestEnableStatistics(
+ cacheName,
+ (cache, b) => cache.Ignite.GetCluster().EnableStatistics(new[] {cacheName}, b));
Review comment:
Please add the following tests additionally:
* Pass empty `cacheNames`
* Pass non-existent cache names
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7434:
IGNITE-7276 .NET: Enable/disable cache statistics in runtime
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7434: IGNITE-7276 .NET: Enable/disable cache statistics in runtime
URL: https://github.com/apache/ignite/pull/7434#discussion_r382923625
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core.Tests/Cache/CacheMetricsTest.cs
##########
@@ -118,6 +120,16 @@ public void TestClusterGroupEnableStatistics()
TestEnableStatistics(
cacheName,
(cache, b) => cache.Ignite.GetCluster().EnableStatistics(new[] {cacheName}, b));
+
+ var ignite = Ignition.GetIgnite();
Review comment:
Can you please extract the code below into two separate tests? Something like:
* `TestClusterEnableStatisticsAllowsEmptyCacheNames`
* `TestClusterEnableStatisticsThrowsOnInvalidCacheName`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7434:
IGNITE-7276 .NET: Enable/disable cache statistics in runtime
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7434: IGNITE-7276 .NET: Enable/disable cache statistics in runtime
URL: https://github.com/apache/ignite/pull/7434#discussion_r380322241
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Cache/CacheImpl.cs
##########
@@ -1173,6 +1173,12 @@ public ICacheMetrics GetLocalMetrics()
});
}
+ /** <inheritDoc /> */
+ public void EnableStatistics(bool enabled)
+ {
+ DoOutInOp((int) CacheOp.EnableStatistics, enabled ? 1 : 0);
Review comment:
`enabled ? True : False`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7434:
IGNITE-7276 .NET: Enable/disable cache statistics in runtime
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7434: IGNITE-7276 .NET: Enable/disable cache statistics in runtime
URL: https://github.com/apache/ignite/pull/7434#discussion_r380321815
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core.Tests/Cache/CacheMetricsTest.cs
##########
@@ -99,6 +99,28 @@ public void TestGlobalMetrics()
Assert.AreEqual(1, remoteMetrics.CachePuts);
}
+ /// <summary>
+ /// Tests the cache metrics enable/disable
+ /// </summary>
+ [Test]
+ public void TestCacheEnableStatistics()
+ {
+ TestEnableStatistics("cacheEnableStatistics", (cache, b) => cache.EnableStatistics(b));
+ }
+
Review comment:
Redundant blank line
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7434:
IGNITE-7276 .NET: Enable/disable cache statistics in runtime
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7434: IGNITE-7276 .NET: Enable/disable cache statistics in runtime
URL: https://github.com/apache/ignite/pull/7434#discussion_r380320124
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/cache/PlatformCache.java
##########
@@ -1174,6 +1177,11 @@ private void loadCache0(BinaryRawReaderEx reader, boolean loc) {
case OP_PRELOAD_PARTITION:
cache.preloadPartition((int)val);
+ return TRUE;
+
+ case OP_ENABLE_STATISTICS:
+ cache.enableStatistics(val == FALSE ? false : true);
Review comment:
`val == FALSE ? false : true` -> `val == 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7434:
IGNITE-7276 .NET: Enable/disable cache statistics in runtime
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7434: IGNITE-7276 .NET: Enable/disable cache statistics in runtime
URL: https://github.com/apache/ignite/pull/7434#discussion_r380322511
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Ignite.cs
##########
@@ -639,6 +639,11 @@ public IServices GetServices()
return _prj.ForServers().GetServices();
}
+ public void EnableStatistics(IEnumerable<string> cacheNames, bool enabled)
Review comment:
Please add `/** <inheritdoc /> */`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7434:
IGNITE-7276 .NET: Enable/disable cache statistics in runtime
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7434: IGNITE-7276 .NET: Enable/disable cache statistics in runtime
URL: https://github.com/apache/ignite/pull/7434#discussion_r382923670
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core.Tests/Cache/CacheMetricsTest.cs
##########
@@ -118,6 +120,16 @@ public void TestClusterGroupEnableStatistics()
TestEnableStatistics(
cacheName,
(cache, b) => cache.Ignite.GetCluster().EnableStatistics(new[] {cacheName}, b));
+
+ var ignite = Ignition.GetIgnite();
+ ignite.CreateCache<int, int>(new CacheConfiguration("clusterEnableStatsValidName"));
+ var cluster = ignite.GetCluster();
+
+ Assert.DoesNotThrow(() => cluster.EnableStatistics(Enumerable.Empty<string>(), true));
+
+ // Nonexistent cache name
+ Assert.Throws<IgniteException>(
Review comment:
Let's add an assertion for the exception message. Does it make sense, is it clear enough for the user to fix the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7434:
IGNITE-7276 .NET: Enable/disable cache statistics in runtime
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7434: IGNITE-7276 .NET: Enable/disable cache statistics in runtime
URL: https://github.com/apache/ignite/pull/7434#discussion_r380321344
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/cluster/PlatformClusterGroup.java
##########
@@ -356,6 +359,22 @@ public PlatformClusterGroup(PlatformContext platformCtx, ClusterGroupEx prj) {
return TRUE;
}
+ case OP_ENABLE_STATISTICS: {
+ boolean enabled = reader.readBoolean();
+
+ int cnt = reader.readInt();
+
+ Collection<String> cacheNames = new ArrayList<>(cnt);
+
+ for (int i = 0; i < cnt; i++) {
+ cacheNames.add(reader.readString());
+ }
+
+ platformCtx.kernalContext().cache().enableStatistics(cacheNames, enabled);
Review comment:
Please use public API when possible: `platformCtx.kernalContext().grid().cluster().enableStatistics(cacheNames, enabled)`
There is a difference in exception handling and `GridKernalGateway.readLock` call.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn merged pull request #7434: IGNITE-7276 .NET:
Enable/disable cache statistics in runtime
Posted by GitBox <gi...@apache.org>.
ptupitsyn merged pull request #7434: IGNITE-7276 .NET: Enable/disable cache statistics in runtime
URL: https://github.com/apache/ignite/pull/7434
----------------------------------------------------------------
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
With regards,
Apache Git Services