Uploaded image for project: 'Maven'
  1. Maven
  2. MNG-5977

Improve output readability of our MavenTransferListener implementations

    Details

      Description

      The current output of Downloading/Downladed/Uploading/Uploaded transfer notification has some flaws:

      1. It does not scale numbers between 1 and 1000 with appropriate units
      2. It should use correct size (kB, MB, GB and time units (s) but doesn't. (see https://en.wikipedia.org/wiki/Binary_prefix and https://en.wikipedia.org/wiki/Metric_prefix)
      3. When Aether downloads in parallel (which applies for non-POM files) the progress interleaves due to race conditions to System.out and you do not know to which resource a progress belongs to.

      Let's use an improved version of MPIR DependenciesRenderer's FileDecimalFormat for it.

      concrete examples:
      before

      191/191 KB   27/48 KB   48/119 KB   80/87 KB   13/13 KB   

      after:

      Progress (4): 500/800 B | 40/45 kB | 193 kB/315 kB | 1.3/9.0 MB | 12/30 MB

      if total size is unavailable or the file has already been downloaded but not removed from the list, the output will be:

      Progress (4): 800 B | 40/45 kB | 193 kB | 9.0 MB | 12 MB

      or in debug mode:

      Progress (5): xml-apis-1.3.04.jar (<progress>) | maven-shared-utils-0.6.jar (<progress>) | xercesImpl-2.9.1.jar (<progress>) | commons-digester-1.6.jar (<progress>) | maven-reporting-impl-2.3.jar (<progress>)

      If the scale is between 1 and 10, one decimal place will be printed out. If it is between 10 and 1000+, it will be an integer.

      1. current.zip
        78 kB
        Michael Osipov
      2. improved.zip
        78 kB
        Michael Osipov
      3. improved-debug.zip
        516 kB
        Michael Osipov

        Issue Links

          Activity

          Hide
          hboutemy Hervé Boutemy added a comment -

          can you give some concrete examples of the issue and the proposed solution output, please?

          Show
          hboutemy Hervé Boutemy added a comment - can you give some concrete examples of the issue and the proposed solution output, please?
          Hide
          michael-o Michael Osipov added a comment - - edited

          I will provide you some today. I have already some code locally on my machine which looks good. I am still trying to solve the parallel write interleave to System.out. Can't figure out what I need to synchronize, out of lineLength.

          Show
          michael-o Michael Osipov added a comment - - edited I will provide you some today. I have already some code locally on my machine which looks good. I am still trying to solve the parallel write interleave to System.out . Can't figure out what I need to synchronize, out of lineLength .
          Hide
          michael-o Michael Osipov added a comment - - edited

          Hervé, here are the two files for console output. The current fix addresses point 1 and 2. It also reduces information when a file has already been downloaded/uploaded but still in the pipeline.

          I am afraid, I have to fix this issue in two steps due to the complexity. The first is already locally fixed, the second one is a concurrency issue: The write operations to the PrintStream aren't synchronized but should be. Since there is no guarantee that your line will properly written without been overridden by another concurrent call. I have observed this during improvement. Addtionally, the writes to the lastLineLength aren't synchronized too. One would probably need to use a ReentrantLock (I will check wether I can find a problem with it at all). Moreover, if a parallel download happens you basically do not know which progress is which. I want to print out filename: xx kB/yy kB filename: xx kB/yy kB etc. since this might get longer as one download/upload URL itself, proper blank filling has to be applied. Here is a good reference.

          Show
          michael-o Michael Osipov added a comment - - edited Hervé, here are the two files for console output. The current fix addresses point 1 and 2. It also reduces information when a file has already been downloaded/uploaded but still in the pipeline. I am afraid, I have to fix this issue in two steps due to the complexity. The first is already locally fixed, the second one is a concurrency issue: The write operations to the PrintStream aren't synchronized but should be. Since there is no guarantee that your line will properly written without been overridden by another concurrent call. I have observed this during improvement. Addtionally, the writes to the lastLineLength aren't synchronized too. One would probably need to use a ReentrantLock (I will check wether I can find a problem with it at all). Moreover, if a parallel download happens you basically do not know which progress is which. I want to print out filename: xx kB/yy kB filename: xx kB/yy kB etc. since this might get longer as one download/upload URL itself, proper blank filling has to be applied. Here is a good reference.
          Hide
          michael-o Michael Osipov added a comment - - edited

          Salut Hervé,

          I have made some progress on the issue and have produced two versions available from: http://home.apache.org/~michaelo/MNG-5977/

          1. apache-maven-3.4.0-MNG-5977-SNAPSHOT-bin.tar.gz: Nicely prints the progress. No race conditions, no syncs where necessary. I have already cleanup superfluous padding and CRs. All looks fine.
          2. apache-maven-3.4.0-MNG-5977-w-filename-SNAPSHOT-bin.tar.gz: Extends the previous implementation by adding the filename to the progress meter. Very nice to read the progress now. The downside is that all methods writing to PrintStream require synchronization otherwise the race conditions will garble the output on long progress lines. (This can be improved by using a ReentrantLock around lastLength and out access only.) There is one glitch I do not understand and don't know how to fix it yet. Under high concurrency, the progress is written but not overwritten by the next progress output or an upcoming "Downloaded:" line. The output written by tee is fine. This fails in cmd and Git bash. This may be a race condition within the Aether threads (unlikely hopefully) or a flaw in Windows or some important flush I forgot. Anyway, I would appreciate if you could take a look at my patches, maybe you will spot the issue. The second patch requires the first one first to be applied.

          In any case, I'd like to prepend the progress meter with something like "In Progress: " or "In Transfer: ". Makes is better readable.
          Tested under Windows 10 only, Linux and FreeBSD is pending.

          Show
          michael-o Michael Osipov added a comment - - edited Salut Hervé, I have made some progress on the issue and have produced two versions available from: http://home.apache.org/~michaelo/MNG-5977/ 1. apache-maven-3.4.0- MNG-5977 -SNAPSHOT-bin.tar.gz : Nicely prints the progress. No race conditions, no syncs where necessary. I have already cleanup superfluous padding and CRs . All looks fine. 2. apache-maven-3.4.0- MNG-5977 -w-filename-SNAPSHOT-bin.tar.gz : Extends the previous implementation by adding the filename to the progress meter. Very nice to read the progress now. The downside is that all methods writing to PrintStream require synchronization otherwise the race conditions will garble the output on long progress lines. (This can be improved by using a ReentrantLock around lastLength and out access only.) There is one glitch I do not understand and don't know how to fix it yet. Under high concurrency, the progress is written but not overwritten by the next progress output or an upcoming "Downloaded:" line. The output written by tee is fine. This fails in cmd and Git bash. This may be a race condition within the Aether threads (unlikely hopefully) or a flaw in Windows or some important flush I forgot. Anyway, I would appreciate if you could take a look at my patches, maybe you will spot the issue. The second patch requires the first one first to be applied. In any case, I'd like to prepend the progress meter with something like "In Progress: " or "In Transfer: ". Makes is better readable. Tested under Windows 10 only, Linux and FreeBSD is pending.
          Hide
          michael-o Michael Osipov added a comment -

          Update on the synchronized issue: surprisingly, all write operations of PrintStream are synchronized in the OpenJDK. This would mean that I could remove the keywords and synchronize only where lastLength is accessed (ro or rw).

          Show
          michael-o Michael Osipov added a comment - Update on the synchronized issue: surprisingly, all write operations of PrintStream are synchronized in the OpenJDK. This would mean that I could remove the keywords and synchronize only where lastLength is accessed ( ro or rw ).
          Hide
          michael-o Michael Osipov added a comment - - edited

          Fixed with ea73fcdb6c0d2bafad75085a38906fcccfa1c049.

          Almost everything fixed and found all issues. The commit message has a prolonged explanation.

          Filename has been omitted from the progress meter for the moment. This line tends to get long with parallel downloads and looks broken (as previously mentioned). This problem is neither Maven not Java but simply the default settings of a terminal. The buffer size of columns is too small which chops the content as well as the carriage return. Though, the filename makes it way better. It can be added if this issue can be accepted.

          Show
          michael-o Michael Osipov added a comment - - edited Fixed with ea73fcdb6c0d2bafad75085a38906fcccfa1c049 . Almost everything fixed and found all issues. The commit message has a prolonged explanation. Filename has been omitted from the progress meter for the moment. This line tends to get long with parallel downloads and looks broken (as previously mentioned). This problem is neither Maven not Java but simply the default settings of a terminal. The buffer size of columns is too small which chops the content as well as the carriage return. Though, the filename makes it way better. It can be added if this issue can be accepted.
          Hide
          michael-o Michael Osipov added a comment -

          If someone can provide a less locking implementation, please provide a patch.

          Show
          michael-o Michael Osipov added a comment - If someone can provide a less locking implementation, please provide a patch.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in maven-3.x #1221 (See https://builds.apache.org/job/maven-3.x/1221/)
          MNG-5977 Improve output readability of our MavenTransferListener (michaelo: rev ea73fcdb6c0d2bafad75085a38906fcccfa1c049)

          • maven-embedder/src/main/java/org/apache/maven/cli/transfer/Slf4jMavenTransferListener.java
          • maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java
          • maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in maven-3.x #1221 (See https://builds.apache.org/job/maven-3.x/1221/ ) MNG-5977 Improve output readability of our MavenTransferListener (michaelo: rev ea73fcdb6c0d2bafad75085a38906fcccfa1c049) maven-embedder/src/main/java/org/apache/maven/cli/transfer/Slf4jMavenTransferListener.java maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java
          Hide
          michael-o Michael Osipov added a comment -

          Adde commit 5efbfd780e5888febfc644582d201f0145246b90 with slight improvements.

          Show
          michael-o Michael Osipov added a comment - Adde commit 5efbfd780e5888febfc644582d201f0145246b90 with slight improvements.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in maven-3.x #1223 (See https://builds.apache.org/job/maven-3.x/1223/)
          MNG-5977 Improve output readability of our MavenTransferListener (michaelo: rev 5efbfd780e5888febfc644582d201f0145246b90)

          • maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in maven-3.x #1223 (See https://builds.apache.org/job/maven-3.x/1223/ ) MNG-5977 Improve output readability of our MavenTransferListener (michaelo: rev 5efbfd780e5888febfc644582d201f0145246b90) maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in maven-3.x #1225 (See https://builds.apache.org/job/maven-3.x/1225/)
          MNG-5977 Improve output readability of our MavenTransferListener (michaelo: rev c5891c10563faa6d76ad6a672abb8c77a3cde94c)

          • maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java
          • maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in maven-3.x #1225 (See https://builds.apache.org/job/maven-3.x/1225/ ) MNG-5977 Improve output readability of our MavenTransferListener (michaelo: rev c5891c10563faa6d76ad6a672abb8c77a3cde94c) maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
          Hide
          michael-o Michael Osipov added a comment -

          Issue needs refinement in code and description.

          Show
          michael-o Michael Osipov added a comment - Issue needs refinement in code and description.
          Hide
          michael-o Michael Osipov added a comment -

          Another improvement has been added with 33442c212b5e5c29feb0101d70a625cea7845ea7.

          Show
          michael-o Michael Osipov added a comment - Another improvement has been added with 33442c212b5e5c29feb0101d70a625cea7845ea7 .
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in maven-3.x #1247 (See https://builds.apache.org/job/maven-3.x/1247/)
          MNG-5977 Improve output readability of our MavenTransferListener (michaelo: rev 33442c212b5e5c29feb0101d70a625cea7845ea7)

          • maven-embedder/src/test/java/org/apache/maven/cli/transfer/FileSizeFormatTest.java
          • maven-embedder/src/main/java/org/apache/maven/cli/transfer/Slf4jMavenTransferListener.java
          • maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java
          • maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in maven-3.x #1247 (See https://builds.apache.org/job/maven-3.x/1247/ ) MNG-5977 Improve output readability of our MavenTransferListener (michaelo: rev 33442c212b5e5c29feb0101d70a625cea7845ea7) maven-embedder/src/test/java/org/apache/maven/cli/transfer/FileSizeFormatTest.java maven-embedder/src/main/java/org/apache/maven/cli/transfer/Slf4jMavenTransferListener.java maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java
          Hide
          michael-o Michael Osipov added a comment -

          Changes in detail:

          • Introduced a FileSizeFormat which flexibly scales the input to the nearest unit (B, kB, MB, GB)
            from 1 to 10 with one decimal place, from 10 with whole numbers. All units cleanly scale with SI prefixes.
            Additionally, a progress format is also available which scales progressed size and total size
            with the unit of the total size. An unknown total size is handled too. If progressed size and total
            size are equal (download has finished), numbers collapse. The format class has full test coverage.
            All transfer listeners use this format equally.
          • Synchronize all calls of the console transfer listener to avoid race conditions for the PrintStream
            which happens for longer lines.
          • Print overriding blank lines when necessary only.
          • Prepend the progress with {{Progress : }} to denote that first progress is going on and second how many
            transfers are in the pipeline.
          • Separate parallel progresses with {{ | }}.
          • Replaced ConcurrentHashMap with a synchronized LinkedHashMap. This retains the insertion
            order of the transfers which makes the progress meter look like a parallel progress queue. No disturbing
            jumps between progress changes anymore.
          • If running in debug mode, the filename is printed additionally.
          Show
          michael-o Michael Osipov added a comment - Changes in detail: Introduced a FileSizeFormat which flexibly scales the input to the nearest unit (B, kB, MB, GB) from 1 to 10 with one decimal place, from 10 with whole numbers. All units cleanly scale with SI prefixes. Additionally, a progress format is also available which scales progressed size and total size with the unit of the total size. An unknown total size is handled too. If progressed size and total size are equal (download has finished), numbers collapse. The format class has full test coverage. All transfer listeners use this format equally. Synchronize all calls of the console transfer listener to avoid race conditions for the PrintStream which happens for longer lines. Print overriding blank lines when necessary only. Prepend the progress with {{Progress : }} to denote that first progress is going on and second how many transfers are in the pipeline. Separate parallel progresses with {{ | }}. Replaced ConcurrentHashMap with a synchronized LinkedHashMap . This retains the insertion order of the transfers which makes the progress meter look like a parallel progress queue. No disturbing jumps between progress changes anymore. If running in debug mode, the filename is printed additionally.
          Hide
          michael-o Michael Osipov added a comment -

          Here are two log files containing output after applied improvements.

          Show
          michael-o Michael Osipov added a comment - Here are two log files containing output after applied improvements.
          Hide
          stephenc Stephen Connolly added a comment -

          Maven 3.4.0 has been dropped. See this thread for more details.

          This issue will need to be re-scheduled for a Maven release in the (hopefully near) future.

          Show
          stephenc Stephen Connolly added a comment - Maven 3.4.0 has been dropped. See this thread for more details. This issue will need to be re-scheduled for a Maven release in the (hopefully near) future.
          Hide
          michael-o Michael Osipov added a comment -
          Show
          michael-o Michael Osipov added a comment - Fixed with 149cce7a867956efeaf72d527f61297bf2471b1e .
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Jenkins build maven-3.x #1501 (See https://builds.apache.org/job/maven-3.x/1501/)
          MNG-5977 Improve output readability of our MavenTransferListener (michaelo: http://git-wip-us.apache.org/repos/asf/?p=maven.git&a=commit&h=149cce7a867956efeaf72d527f61297bf2471b1e)

          • (edit) maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
          • (edit) maven-embedder/src/main/java/org/apache/maven/cli/transfer/Slf4jMavenTransferListener.java
          • (edit) maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java
          • (edit) maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java
          • (add) maven-embedder/src/test/java/org/apache/maven/cli/transfer/FileSizeFormatTest.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Jenkins build maven-3.x #1501 (See https://builds.apache.org/job/maven-3.x/1501/ ) MNG-5977 Improve output readability of our MavenTransferListener (michaelo: http://git-wip-us.apache.org/repos/asf/?p=maven.git&a=commit&h=149cce7a867956efeaf72d527f61297bf2471b1e ) (edit) maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java (edit) maven-embedder/src/main/java/org/apache/maven/cli/transfer/Slf4jMavenTransferListener.java (edit) maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java (edit) maven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java (add) maven-embedder/src/test/java/org/apache/maven/cli/transfer/FileSizeFormatTest.java

            People

            • Assignee:
              michael-o Michael Osipov
              Reporter:
              michael-o Michael Osipov
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development