You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/07/08 11:03:44 UTC
[GitHub] [maven-surefire] rmannibucau opened a new pull request #305: enable to flush regularly output of the forked process
rmannibucau opened a new pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305
Workaround for M5 regression on test outputs. Enables to configure a flushing interval in surefire config:
<outputFlushInterval>250</outputFlushInterval>
This enables to stay interactive when working with tests with surefire and not await for minutes when test is slow (IT, E2E)
----------------------------------------------------------------
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] [maven-surefire] eolivelli commented on a change in pull request #305: enable to flush regularly output of the forked process
Posted by 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
[GitHub] [maven-surefire] rmannibucau commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-655485234
@Tibor17 I did it this way to respect the abstraction we have and the way surefire handles the lifecycle of the stream. It is the only shared place for all encoders so where it should be done to avoid to have to duplicate it in all impl which would be way more expensive in terms of maintenance cost IMHO.
----------------------------------------------------------------
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] [maven-surefire] rmannibucau commented on a change in pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#discussion_r451510317
##########
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:
this is a SPI (so user facing) so didn't want to modify it since this is really a workaround :s
----------------------------------------------------------------
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] [maven-surefire] Tibor17 commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-660949560
@rmannibucau
@eolivelli
Hye guys. Can you please make a code review for the [commit](https://github.com/apache/maven-surefire/commit/4b4eaf68baa64de6182ab49f8c1984e6da789da0)? It is the fix for this flush problem. I made a manual test 5 console lines and 320 thousands console lines too, and compared the behavior against the previous version. You will see optimization with calling the flush. Without it i felt the console was not that terribly fast as it is 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] rmannibucau commented on a change in pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#discussion_r451508567
##########
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:
Hope is that it gets removed in 3.0.0 and does not change default behavior so used 0 (disable) as default.
----------------------------------------------------------------
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] [maven-surefire] rmannibucau commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-655487869
@Tibor17 if what you are saying is true it means we don't need all these abstraction and user can't plug that so let's drop all the code. Also note that this PR does not enable it by default so it does not impact at all all the cases you reference.
----------------------------------------------------------------
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] [maven-surefire] eolivelli commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-660956655
@Tibor17
I left a minor comment on your PR.
It is better that you open a new PR, this way it is easier to track comments and discussions.
----------------------------------------------------------------
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] [maven-surefire] Tibor17 edited a comment on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-655486824
No it is not duplicate. It is polymorphism, so every impl may have another fix. And btw you know very well that a shared code can be always moved to a shared class, some helper class or whatever we call it, i don't care. Maybe some File I/O would not need to have a thread and that the benefit of the polymorphism, but this way in the PR provider/connector has to consider 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-655486824
No it is no duplicate. It is polymorphism, so every impl may have another fix. And btw you know very well that a shared code can be always moved to a shared class, some helper class or whatever we call it, i don't care. Maybe some File I/O would not need to have a thread and that the benefit of the polymorphism, but this way in the PR provider/connector has to consider 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] rmannibucau closed pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
rmannibucau closed pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-655501393
There are two places where this change should reside and we did not have to touch everything to make it configurable. The users did not care till now and they do not care. They only want to have it working.
This is overhead!
Unnecessary overhead and spaghetti code.
----------------------------------------------------------------
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] [maven-surefire] rmannibucau commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-655548750
@Tibor17 if you make the flushing timeout configurable (or at least > 100ms) and await the executor is shut down before exiting I'm fine with your fix too
----------------------------------------------------------------
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] [maven-surefire] eolivelli commented on a change in pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#discussion_r451512866
##########
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:
I see
----------------------------------------------------------------
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] [maven-surefire] Tibor17 commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-656630572
The branch https://github.com/apache/maven-surefire/commits/flush should be tested with a real project.
It has wrong order of commits. So it has to be refactored.
Currently, i am waiting for the test "junit47-twoForks-ff1" because it sometimes fails in whatever feature and now it is annoying. The test ATest and BTest should run in parallel in two forks but it looks like they are not parallel. The logs will show us more.
----------------------------------------------------------------
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] [maven-surefire] Tibor17 commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-655482913
@rmannibucau
Why you did it so complicated, the spaghetti code. Now we have to maintain this feature in several places.
There is an abstrcation and the purpose it it is to make these non-functional changes hidden in the implementation. These two places are called only once, so it is easy just to put a Thread here and call the flush() method:
https://github.com/apache/maven-surefire/blob/master/surefire-booter/src/main/java/org/apache/maven/surefire/booter/spi/LegacyMasterProcessChannelProcessorFactory.java#L65
https://github.com/apache/maven-surefire/blob/master/surefire-booter/src/main/java/org/apache/maven/surefire/booter/spi/SurefireMasterProcessChannelProcessorFactory.java#L107
----------------------------------------------------------------
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] [maven-surefire] eolivelli commented on a change in pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#discussion_r451513375
##########
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:
I see.
The API is still unstable, maybe it is better to change it now.
I am fine with keeping this trick here as an hotfix.
but we should create a JIRA at least to not forget
----------------------------------------------------------------
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] [maven-surefire] eolivelli commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-655539640
@Tibor17 your fix works as well for me
https://github.com/apache/maven-surefire/commit/50bc56d4f76c726cbbb472cd98bece268cf38a7c
----------------------------------------------------------------
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] [maven-surefire] Tibor17 commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-661052630
@eolivelli
@rmannibucau
welcome to the PR #308 .
----------------------------------------------------------------
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] [maven-surefire] Tibor17 commented on pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#issuecomment-655526242
@rmannibucau
@olamy
@eolivelli
See this simple solution. One class and everything is hidden. The flush after 100 millis.
https://github.com/apache/maven-surefire/commit/50bc56d4f76c726cbbb472cd98bece268cf38a7c
----------------------------------------------------------------
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] [maven-surefire] rmannibucau commented on a change in pull request #305: enable to flush regularly output of the forked process
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #305:
URL: https://github.com/apache/maven-surefire/pull/305#discussion_r451509023
##########
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:
For the same reason than the default is 0, I didn't want to modify the API there for this (non-)feature so instanceof was the best compromise IMHO even if not as elegant as adding 2 methods.
----------------------------------------------------------------
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