Address some of Terry's comments.

Andrew Fogarty · Andrew Fogarty · commit 70404d44c587 · 2020-10-03T11:48:49.000-07:00
diff --git a/src/csharp/Microsoft.Spark.E2ETest/IpcTests/JvmThreadPoolGCTests.cs b/src/csharp/Microsoft.Spark.E2ETest/IpcTests/JvmThreadPoolGCTests.cs
@@ -12,12 +12,12 @@
 namespace Microsoft.Spark.E2ETest.IpcTests
 {
     [Collection("Spark E2E Tests")]
-    public class JvmThreadPoolGarbageCollectorTests
+    public class JvmThreadPoolGCTests
     {
         private readonly SparkSession _spark;
         private readonly IJvmBridge _jvmBridge;
 
-        public JvmThreadPoolGarbageCollectorTests(SparkFixture fixture)
+        public JvmThreadPoolGCTests(SparkFixture fixture)
         {
             _spark = fixture.Spark;
             _jvmBridge = ((IJvmObjectReferenceProvider)_spark).Reference.Jvm;
@@ -42,7 +42,7 @@ void testChildThread(string appName)
 
                     // Since we are in the child thread, GetActiveSession() should return the child
                     // SparkSession.
-                    var activeSession = SparkSession.GetActiveSession();
+                    SparkSession activeSession = SparkSession.GetActiveSession();
                     Assert.NotNull(activeSession);
                     Assert.Equal(appName, activeSession.Conf().Get("spark.app.name", null));
                 });
@@ -51,7 +51,7 @@ void testChildThread(string appName)
                 thread.Join();
             }
 
-            for (var i = 0; i < 5; i++)
+            for (var i = 0; i < 5; ++i)
             {
                 testChildThread(i.ToString());
             }
@@ -60,12 +60,12 @@ void testChildThread(string appName)
         }
 
         /// <summary>
-        /// Monitor a thread via the JvmThreadPoolGarbageCollector.
+        /// Monitor a thread via the JvmThreadPoolGC.
         /// </summary>
         [Fact]
-        public void TestMonitorThread()
+        public void TestTryAddThread()
         {
-            var threadPool = new JvmThreadPoolGarbageCollector(_jvmBridge, TimeSpan.FromMinutes(30));
+            using var threadPool = new JvmThreadPoolGC(_jvmBridge, TimeSpan.FromMinutes(30));
 
             var thread = new Thread(() => _spark.Sql("SELECT TRUE"));
             thread.Start();
@@ -92,19 +92,23 @@ public void TestThreadRm()
         }
 
         /// <summary>
-        /// Test that the JvmThreadGarbageCollectionInterval configuration defaults to 5 minutes,
-        /// and can be updated correctly by setting the environment variable.
+        /// Test that the GC interval configuration defaults to 5 minutes, and can be updated
+        /// correctly by setting the environment variable.
         /// </summary>
         [Fact]
         public void TestIntervalConfiguration()
         {
             // Default value is 5 minutes.
             Assert.Null(Environment.GetEnvironmentVariable("DOTNET_THREAD_GC_INTERVAL"));
-            Assert.Equal(TimeSpan.FromMinutes(5), SparkEnvironment.ConfigurationService.JvmThreadGarbageCollectionInterval);
+            Assert.Equal(
+                TimeSpan.FromMinutes(5),
+                SparkEnvironment.ConfigurationService.JvmThreadGCInterval);
 
             // Test a custom value.
             Environment.SetEnvironmentVariable("DOTNET_THREAD_GC_INTERVAL", "1:30:00");
-            Assert.Equal(TimeSpan.FromMinutes(90), SparkEnvironment.ConfigurationService.JvmThreadGarbageCollectionInterval);
+            Assert.Equal(
+                TimeSpan.FromMinutes(90),
+                SparkEnvironment.ConfigurationService.JvmThreadGCInterval);
         }
     }
 }
diff --git a/src/csharp/Microsoft.Spark/Interop/Ipc/JvmBridge.cs b/src/csharp/Microsoft.Spark/Interop/Ipc/JvmBridge.cs
@@ -36,7 +36,7 @@ internal sealed class JvmBridge : IJvmBridge
         private readonly ILoggerService _logger =
             LoggerServiceFactory.GetLogger(typeof(JvmBridge));
         private readonly int _portNumber;
-        private readonly JvmThreadPoolGarbageCollector _jvmThreadPool;
+        private readonly JvmThreadPoolGC _jvmThreadPoolGC;
 
         internal JvmBridge(int portNumber)
         {
@@ -48,8 +48,8 @@ internal JvmBridge(int portNumber)
             _portNumber = portNumber;
             _logger.LogInfo($"JvMBridge port is {portNumber}");
 
-            _jvmThreadPool = new JvmThreadPoolGarbageCollector(
-                this, SparkEnvironment.ConfigurationService.JvmThreadGarbageCollectionInterval);
+            _jvmThreadPoolGC = new JvmThreadPoolGC(
+                this, SparkEnvironment.ConfigurationService.JvmThreadGCInterval);
         }
 
         private ISocketWrapper GetConnection()
@@ -183,7 +183,7 @@ private object CallJavaMethod(
                     (int)payloadMemoryStream.Position);
                 outputStream.Flush();
 
-                _jvmThreadPool.TryAddThread(thread);
+                _jvmThreadPoolGC.TryAddThread(thread);
 
                 Stream inputStream = socket.InputStream;
                 int isMethodCallFailed = SerDe.ReadInt32(inputStream);
@@ -419,7 +419,7 @@ private object ReadCollection(Stream s)
 
         public void Dispose()
         {
-            _jvmThreadPool.Dispose();
+            _jvmThreadPoolGC.Dispose();
             while (_sockets.TryDequeue(out ISocketWrapper socket))
             {
                 if (socket != null)
diff --git a/src/csharp/Microsoft.Spark/Interop/Ipc/JvmThreadPoolGC.cs b/src/csharp/Microsoft.Spark/Interop/Ipc/JvmThreadPoolGC.cs
@@ -19,7 +19,7 @@ namespace Microsoft.Spark.Interop.Ipc
     /// thread is no longer alive, this class submits an rmThread command to the JVM backend to 
     /// dispose of its corresponding JVM thread. All methods are thread-safe.
     /// </summary>
-    internal class JvmThreadPoolGarbageCollector : IDisposable
+    internal class JvmThreadPoolGC : IDisposable
     {
         private readonly IJvmBridge _jvmBridge;
         private readonly TimeSpan _threadGCInterval;
@@ -29,11 +29,11 @@ internal class JvmThreadPoolGarbageCollector : IDisposable
         private Timer _activeThreadGCTimer;
 
         /// <summary>
-        /// Construct the JvmThreadPoolGarbageCollector.
+        /// Construct the JvmThreadPoolGC.
         /// </summary>
         /// <param name="jvmBridge">The JvmBridge used to call JVM methods.</param>
         /// <param name="threadGCInterval">The interval to GC finished threads.</param>
-        public JvmThreadPoolGarbageCollector(IJvmBridge jvmBridge, TimeSpan threadGCInterval)
+        public JvmThreadPoolGC(IJvmBridge jvmBridge, TimeSpan threadGCInterval)
         {
             _jvmBridge = jvmBridge;
             _threadGCInterval = threadGCInterval;
@@ -57,7 +57,7 @@ public void Dispose()
                 }
             }
 
-            GarbageCollectThreads();
+            GCThreads();
         }
 
         /// <summary>
@@ -77,7 +77,7 @@ public bool TryAddThread(Thread thread)
                     if (_activeThreadGCTimer == null && _activeThreads.Count > 0)
                     {
                         _activeThreadGCTimer = new Timer(
-                            (state) => GarbageCollectThreads(),
+                            (state) => GCThreads(),
                             null,
                             _threadGCInterval,
                             _threadGCInterval);
@@ -112,7 +112,7 @@ private bool TryRemoveAndDisposeThread(int threadId)
         /// <summary>
         /// Remove any threads that are no longer active.
         /// </summary>
-        private void GarbageCollectThreads()
+        private void GCThreads()
         {
             foreach (KeyValuePair<int, Thread> kvp in _activeThreads)
             {
diff --git a/src/csharp/Microsoft.Spark/Services/ConfigurationService.cs b/src/csharp/Microsoft.Spark/Services/ConfigurationService.cs
@@ -34,9 +34,9 @@ internal sealed class ConfigurationService : IConfigurationService
         private string _workerPath;
 
         /// <summary>
-        /// How often to run GC on JVM ThreadPool threads.
+        /// How often to run GC on JVM ThreadPool threads. Defaults to 5 minutes.
         /// </summary>
-        public TimeSpan JvmThreadGarbageCollectionInterval
+        public TimeSpan JvmThreadGCInterval
         {
             get
             {
diff --git a/src/csharp/Microsoft.Spark/Services/IConfigurationService.cs b/src/csharp/Microsoft.Spark/Services/IConfigurationService.cs
@@ -12,9 +12,9 @@ namespace Microsoft.Spark.Services
     internal interface IConfigurationService
     {
         /// <summary>
-        /// How often to run GC on JVM ThreadPool threads. Default value is 5 minutes.
+        /// How often to run GC on JVM ThreadPool threads.
         /// </summary>
-        TimeSpan JvmThreadGarbageCollectionInterval { get; }
+        TimeSpan JvmThreadGCInterval { get; }
 
         /// <summary>
         /// The port number used for communicating with the .NET backend process.

Original file line number	Diff line number	Diff line change
`@@ -12,12 +12,12 @@`
`12`	`12`	`namespace Microsoft.Spark.E2ETest.IpcTests`
`13`	`13`	`{`
`14`	`14`	`[Collection("Spark E2E Tests")]`
`15`		`- public class JvmThreadPoolGarbageCollectorTests`
	`15`	`+ public class JvmThreadPoolGCTests`
`16`	`16`	`{`
`17`	`17`	`private readonly SparkSession _spark;`
`18`	`18`	`private readonly IJvmBridge _jvmBridge;`
`19`	`19`
`20`		`- public JvmThreadPoolGarbageCollectorTests(SparkFixture fixture)`
	`20`	`+ public JvmThreadPoolGCTests(SparkFixture fixture)`
`21`	`21`	`{`
`22`	`22`	`_spark = fixture.Spark;`
`23`	`23`	`_jvmBridge = ((IJvmObjectReferenceProvider)_spark).Reference.Jvm;`
`@@ -42,7 +42,7 @@ void testChildThread(string appName)`
`42`	`42`
`43`	`43`	`// Since we are in the child thread, GetActiveSession() should return the child`
`44`	`44`	`// SparkSession.`
`45`		`- var activeSession = SparkSession.GetActiveSession();`
	`45`	`+ SparkSession activeSession = SparkSession.GetActiveSession();`
`46`	`46`	`Assert.NotNull(activeSession);`
`47`	`47`	`Assert.Equal(appName, activeSession.Conf().Get("spark.app.name", null));`
`48`	`48`	`});`
`@@ -51,7 +51,7 @@ void testChildThread(string appName)`
`51`	`51`	`thread.Join();`
`52`	`52`	`}`
`53`	`53`
`54`		`- for (var i = 0; i < 5; i++)`
	`54`	`+ for (var i = 0; i < 5; ++i)`
`55`	`55`	`{`
`56`	`56`	`testChildThread(i.ToString());`
`57`	`57`	`}`
`@@ -60,12 +60,12 @@ void testChildThread(string appName)`
`60`	`60`	`}`
`61`	`61`
`62`	`62`	`/// <summary>`
`63`		`- /// Monitor a thread via the JvmThreadPoolGarbageCollector.`
	`63`	`+ /// Monitor a thread via the JvmThreadPoolGC.`
`64`	`64`	`/// </summary>`
`65`	`65`	`[Fact]`
`66`		`- public void TestMonitorThread()`
	`66`	`+ public void TestTryAddThread()`
`67`	`67`	`{`
`68`		`- var threadPool = new JvmThreadPoolGarbageCollector(_jvmBridge, TimeSpan.FromMinutes(30));`
	`68`	`+ using var threadPool = new JvmThreadPoolGC(_jvmBridge, TimeSpan.FromMinutes(30));`
`69`	`69`
`70`	`70`	`var thread = new Thread(() => _spark.Sql("SELECT TRUE"));`
`71`	`71`	`thread.Start();`
`@@ -92,19 +92,23 @@ public void TestThreadRm()`
`92`	`92`	`}`
`93`	`93`
`94`	`94`	`/// <summary>`
`95`		`- /// Test that the JvmThreadGarbageCollectionInterval configuration defaults to 5 minutes,`
`96`		`- /// and can be updated correctly by setting the environment variable.`
	`95`	`+ /// Test that the GC interval configuration defaults to 5 minutes, and can be updated`
	`96`	`+ /// correctly by setting the environment variable.`
`97`	`97`	`/// </summary>`
`98`	`98`	`[Fact]`
`99`	`99`	`public void TestIntervalConfiguration()`
`100`	`100`	`{`
`101`	`101`	`// Default value is 5 minutes.`
`102`	`102`	`Assert.Null(Environment.GetEnvironmentVariable("DOTNET_THREAD_GC_INTERVAL"));`
`103`		`- Assert.Equal(TimeSpan.FromMinutes(5), SparkEnvironment.ConfigurationService.JvmThreadGarbageCollectionInterval);`
	`103`	`+ Assert.Equal(`
	`104`	`+ TimeSpan.FromMinutes(5),`
	`105`	`+ SparkEnvironment.ConfigurationService.JvmThreadGCInterval);`
`104`	`106`
`105`	`107`	`// Test a custom value.`
`106`	`108`	`Environment.SetEnvironmentVariable("DOTNET_THREAD_GC_INTERVAL", "1:30:00");`
`107`		`- Assert.Equal(TimeSpan.FromMinutes(90), SparkEnvironment.ConfigurationService.JvmThreadGarbageCollectionInterval);`
	`109`	`+ Assert.Equal(`
	`110`	`+ TimeSpan.FromMinutes(90),`
	`111`	`+ SparkEnvironment.ConfigurationService.JvmThreadGCInterval);`
`108`	`112`	`}`
`109`	`113`	`}`
`110`	`114`	`}`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ namespace Microsoft.Spark.Interop.Ipc`
`19`	`19`	`/// thread is no longer alive, this class submits an rmThread command to the JVM backend to`
`20`	`20`	`/// dispose of its corresponding JVM thread. All methods are thread-safe.`
`21`	`21`	`/// </summary>`
`22`		`- internal class JvmThreadPoolGarbageCollector : IDisposable`
	`22`	`+ internal class JvmThreadPoolGC : IDisposable`
`23`	`23`	`{`
`24`	`24`	`private readonly IJvmBridge _jvmBridge;`
`25`	`25`	`private readonly TimeSpan _threadGCInterval;`
`@@ -29,11 +29,11 @@ internal class JvmThreadPoolGarbageCollector : IDisposable`
`29`	`29`	`private Timer _activeThreadGCTimer;`
`30`	`30`
`31`	`31`	`/// <summary>`
`32`		`- /// Construct the JvmThreadPoolGarbageCollector.`
	`32`	`+ /// Construct the JvmThreadPoolGC.`
`33`	`33`	`/// </summary>`
`34`	`34`	`/// <param name="jvmBridge">The JvmBridge used to call JVM methods.</param>`
`35`	`35`	`/// <param name="threadGCInterval">The interval to GC finished threads.</param>`
`36`		`- public JvmThreadPoolGarbageCollector(IJvmBridge jvmBridge, TimeSpan threadGCInterval)`
	`36`	`+ public JvmThreadPoolGC(IJvmBridge jvmBridge, TimeSpan threadGCInterval)`
`37`	`37`	`{`
`38`	`38`	`_jvmBridge = jvmBridge;`
`39`	`39`	`_threadGCInterval = threadGCInterval;`
`@@ -57,7 +57,7 @@ public void Dispose()`
`57`	`57`	`}`
`58`	`58`	`}`
`59`	`59`
`60`		`- GarbageCollectThreads();`
	`60`	`+ GCThreads();`
`61`	`61`	`}`
`62`	`62`
`63`	`63`	`/// <summary>`
`@@ -77,7 +77,7 @@ public bool TryAddThread(Thread thread)`
`77`	`77`	`if (_activeThreadGCTimer == null && _activeThreads.Count > 0)`
`78`	`78`	`{`
`79`	`79`	`_activeThreadGCTimer = new Timer(`
`80`		`- (state) => GarbageCollectThreads(),`
	`80`	`+ (state) => GCThreads(),`
`81`	`81`	`null,`
`82`	`82`	`_threadGCInterval,`
`83`	`83`	`_threadGCInterval);`
`@@ -112,7 +112,7 @@ private bool TryRemoveAndDisposeThread(int threadId)`
`112`	`112`	`/// <summary>`
`113`	`113`	`/// Remove any threads that are no longer active.`
`114`	`114`	`/// </summary>`
`115`		`- private void GarbageCollectThreads()`
	`115`	`+ private void GCThreads()`
`116`	`116`	`{`
`117`	`117`	`foreach (KeyValuePair<int, Thread> kvp in _activeThreads)`
`118`	`118`	`{`
Original file line number	Diff line number	Diff line change
`@@ -34,9 +34,9 @@ internal sealed class ConfigurationService : IConfigurationService`
`34`	`34`	`private string _workerPath;`
`35`	`35`
`36`	`36`	`/// <summary>`
`37`		`- /// How often to run GC on JVM ThreadPool threads.`
	`37`	`+ /// How often to run GC on JVM ThreadPool threads. Defaults to 5 minutes.`
`38`	`38`	`/// </summary>`
`39`		`- public TimeSpan JvmThreadGarbageCollectionInterval`
	`39`	`+ public TimeSpan JvmThreadGCInterval`
`40`	`40`	`{`
`41`	`41`	`get`
`42`	`42`	`{`