[GitHub] [maven-surefire] eolivelli commented on a change in pull request #305: enable to flush regularly output of the forked process

[GitBox <gi...@apache.org>]

eolivelli commented on a change in pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#discussion_r451505393



##########
File path: surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java
##########
@@ -129,6 +134,34 @@ private void setupBooter( String tmpDir, String dumpFileName, String surefirePro
 
         flushEventChannelOnExit();
 
+        final Long outputFlushInterval = providerConfiguration.getOutputFlushInterval();
+        if ( outputFlushInterval != null && outputFlushInterval > 0
+            && LegacyMasterProcessChannelEncoder.class.isInstance( eventChannel ) )

Review comment:
       instanceof smells a little.
   do we have any more object oriented friendly way ?

##########
File path: surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java
##########
@@ -129,6 +134,34 @@ private void setupBooter( String tmpDir, String dumpFileName, String surefirePro
 
         flushEventChannelOnExit();
 
+        final Long outputFlushInterval = providerConfiguration.getOutputFlushInterval();
+        if ( outputFlushInterval != null && outputFlushInterval > 0
+            && LegacyMasterProcessChannelEncoder.class.isInstance( eventChannel ) )
+        {
+            final LegacyMasterProcessChannelEncoder enc = LegacyMasterProcessChannelEncoder.class.cast( eventChannel );
+            flusher = Executors.newSingleThreadScheduledExecutor( DaemonThreadFactory.newDaemonThreadFactory() );
+            flusher.scheduleAtFixedRate( new Runnable()
+            {
+                @Override
+                public void run()
+                {
+                    final WritableBufferedByteChannel c = WritableBufferedByteChannel.class.cast( enc.getOut() );
+                    try
+                    {
+                        if ( c.isOpen() )

Review comment:
       what about adding a 
   `MasterProcessChannelEncoder#flush() `
   method
   
   this way we can get rid of all of the casts.
   LegacyMasterProcessChannelEncoder will implement it this way, other implementations will not implement it or implement it as an empty method

##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -808,6 +808,18 @@
     @Parameter
     private Map<String, String> jdkToolchain;
 
+    /**
+     * How often output is forced to be flushed.
+     * Useful when there is no output for N ms but some data are buffered.
+     * It will trigger a flush each configured interval to ensure
+     * data don't stay blocked in a buffer.
+     * Setting it to 0 disable that feature.
+     *
+     * @since 3.0.0-M6
+     */
+    @Parameter( defaultValue = "0" )

Review comment:
       why about setting the default to 500ms ?
   otherwise people will add some value to every pom.xml
   if we have a sensible default then we will save users requests on the ML and pom pollution

##########
File path: surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java
##########
@@ -129,6 +134,34 @@ private void setupBooter( String tmpDir, String dumpFileName, String surefirePro
 
         flushEventChannelOnExit();
 
+        final Long outputFlushInterval = providerConfiguration.getOutputFlushInterval();
+        if ( outputFlushInterval != null && outputFlushInterval > 0
+            && LegacyMasterProcessChannelEncoder.class.isInstance( eventChannel ) )
+        {
+            final LegacyMasterProcessChannelEncoder enc = LegacyMasterProcessChannelEncoder.class.cast( eventChannel );
+            flusher = Executors.newSingleThreadScheduledExecutor( DaemonThreadFactory.newDaemonThreadFactory() );
+            flusher.scheduleAtFixedRate( new Runnable()
+            {
+                @Override
+                public void run()
+                {
+                    final WritableBufferedByteChannel c = WritableBufferedByteChannel.class.cast( enc.getOut() );

Review comment:
       can we make this cast inside the main thread, like at line 142 ?
   or is "out" still null at that point ?

##########
File path: surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java
##########
@@ -129,6 +134,34 @@ private void setupBooter( String tmpDir, String dumpFileName, String surefirePro
 
         flushEventChannelOnExit();
 
+        final Long outputFlushInterval = providerConfiguration.getOutputFlushInterval();
+        if ( outputFlushInterval != null && outputFlushInterval > 0
+            && LegacyMasterProcessChannelEncoder.class.isInstance( eventChannel ) )
+        {
+            final LegacyMasterProcessChannelEncoder enc = LegacyMasterProcessChannelEncoder.class.cast( eventChannel );
+            flusher = Executors.newSingleThreadScheduledExecutor( DaemonThreadFactory.newDaemonThreadFactory() );

Review comment:
       can we give a name to this thread ?




----------------------------------------------------------------
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