Create at most one PrintStream per log file. #661

hwellmann · 2016-12-17T17:19:15Z

This avoids corrupted log files.

Fixes #652.

This avoids corrupted log files. Fixes fabric8io#652.

codecov-io · 2016-12-17T17:23:49Z

Current coverage is 48.96% (diff: 59.50%)

Merging #661 into master will increase coverage by 1.59%

@@             master       #661   diff @@
==========================================
  Files           113        123    +10   
  Lines          5881       6333   +452   
  Methods           0          0          
  Messages          0          0          
  Branches       1020       1077    +57   
==========================================
+ Hits           2786       3101   +315   
- Misses         2850       2961   +111   
- Partials        245        271    +26

Diff Coverage	File Path
0%	...cker/access/hc/unix/UnixConnectionSocketFactory.java
0%	...maven/docker/access/hc/DockerAccessWithHcClient.java
0%	...java/io/fabric8/maven/docker/service/ServiceHub.java
0%	new ...n/java/io/fabric8/maven/docker/VolumeRemoveMojo.java
0%	.../fabric8/maven/docker/config/ImageConfiguration.java
0%	new ...n/java/io/fabric8/maven/docker/VolumeCreateMojo.java
0%	new .../fabric8/maven/docker/access/hc/unix/UnixSocket.java
0%	...fig/handler/compose/DockerComposeServiceWrapper.java
0%	...java/io/fabric8/maven/docker/AbstractDockerMojo.java
0%	src/main/java/io/fabric8/maven/docker/StartMojo.java

Review all 28 files changed

Powered by Codecov. Last update f7008c3...b7aaed5

AWS ECR key exchange closes fabric8io#637 Signed-off-by: Chas Honton <chas@apache.org>

Signed-off-by: Chas Honton <chas@apache.org>

PR merged! Thanks!

rhuss

Thanks a lot ! I think we should implement the fix slightly differently, see my comment on LogDispatcher.

If you don't have time to change this this week, I could take this over to fix this.

rhuss · 2016-12-20T07:25:47Z

src/main/java/io/fabric8/maven/docker/log/LogCallbackFactory.java

+        return ps;
+    }
+
+    public static void closeLogs() {


This method should be synchronized, too

rhuss · 2016-12-20T07:28:00Z

src/main/java/io/fabric8/maven/docker/log/LogDispatcher.java

@@ -37,15 +40,16 @@ public LogDispatcher(DockerAccess dockerAccess) {
    }

    public synchronized void trackContainerLog(String containerId, LogOutputSpec spec) throws FileNotFoundException {
-        LogGetHandle handle = dockerAccess.getLogAsync(containerId, new DefaultLogCallback(spec));
+        LogGetHandle handle = dockerAccess.getLogAsync(containerId, createLogCallback(spec));


I'm not a a fan of static imports as they dilute the real locations and I always search first in the current class when doing review. Could we refer to the static method via its class (LogCallbackFactory.createLogBack()) ? Even saves some lines.

rhuss · 2016-12-20T07:39:25Z

src/main/java/io/fabric8/maven/docker/access/hc/DockerAccessWithHcClient.java

@@ -145,7 +146,7 @@ public void startExecContainer(String containerId, LogOutputSpec outputSpec) thr
    }

    private ResponseHandler<Object> createExecResponseHandler(LogOutputSpec outputSpec) throws FileNotFoundException {
-        final LogCallback callback = new DefaultLogCallback(outputSpec);
+        final LogCallback callback = createLogCallback(outputSpec);


Who is closing this DefaultLogCallback() ?

rhuss · 2016-12-20T07:49:22Z

src/main/java/io/fabric8/maven/docker/log/LogDispatcher.java

    }

    public synchronized void untrackAllContainerLogs() {
+        closeLogs();


This will close all streams from all LogDispatcher instances running. Not sure why it could be more than one, but at least we should avoid the mismatch with the LogCallbackFactory storing the stream in a static Map vs. using object instances for the LogDispatcher here. I would prefer to have LogCallbackFactory to be an object, too.

What's about the following idea: We simply allow DefaultLogCallback to reuse an existing printstream (i.e. we do the caching on a static field in DefaultLogCallback along with a usage count) and add a close() method to the callback so that the user can signal that he is done with logging ? In close() an the associated printstream's usage count will be decreased and if 0 then closed.

I think this kind of resource management makes more sense, also for potential future use cases. If you don't have the cycles to implement this, no problem, I could take over (as this is indeed an important fix to have). Also, a unit tests for DefaultLogCallback would be awesome then.

hwellmann · 2016-12-20T12:44:25Z

Thanks for your feedback. I'll get back to this tomorrow...

rhuss · 2016-12-20T18:07:01Z

Awesome,thx unfortunately I just catched an ugly flu, and staying in bed currently. I will continue here asap. Sorry ... Harald Wellmann <notifications@github.com> schrieb am Di., 20. Dez. 2016, 13:44:

…

Thanks for your feedback. I'll get back to this tomorrow... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#661 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGDCF5LZDD0ujqAvS_aVoM9v4dxa36oks5rJ82pgaJpZM4LP7z2> .

Fixes fabric8io#652.

hwellmann · 2016-12-21T15:48:42Z

Reworked DefaultLogCallback with reference counting and added a test class. The LogCallbackFactory is gone.

Note: To test this in the context of my real-life project, I had to revert 7afec26, see #660.

Master appears to be broken, I'm seeing the same issue both on MacOS and Linux.

Signed-off-by: Chas Honton <chas@apache.org>

added ability to explicitly via configuration to create named volumes via <volumes><volume> configuration. Example config ```xml <volumes> <volume> <name>Volume Name</name> <driver>local</driver> <driverOpts> <driverOpt-key-name>driverOpt-value</driverOpt-key-name> </driverOpts> <labels> <label-key>someValue</label-key> <com-github-fabric8-dmp-sample-volume> voluminous </com-github-fabric8-dmp-sample-volume> </labels> </volume> </volumes> ``` Added a new volume sample Added a new goal volume-create minor formatting / spelling correction for intro.md Signed-off-by: Tom Burton <Tom.Burton@Outlook.com>

Signed-off-by: Tom Burton <Tom.Burton@Outlook.com>

fabric8io#664 (review) corrected spelling corrected ascii-doc + formatting code formatting Signed-off-by: Tom Burton <Tom.Burton@Outlook.com>

This is a cleanup of PR fabric8io#664 with the following major points tackled: * Cleaned formatting * Removed unneeded classes and service method for listing and getting volumes. This is not needed now and probablu not in the future. * Fixed some Copy&Pase errors (like the default phase for 'volume-remove' which was the same as for 'volume-create' * Fixed tests. Please note, that *no service method* should be adapted to be "testable". Especially do not introduce a non-private method only for this purpose and test this method instead of the offiical API. Modern Mock frameworks can deal with the the problems which are tried to be solved with this. Otherwise the API gets dilulted, more complicated, more maintenance, more coupled. * Removed assert that do nothing. Tests can be still improved for the service but its fine for now, Next step is to add support for the PropertyHandler to allow configuration of the volumes also via properties.

PR merged! Thanks!

7afec26

PR merged! Thanks!

…ven-plugin into hwellmann-garbled-logs

rhuss

Thanks a lot. I have some minor comments. If you dont mind I would fix them right now in order to proceed for a next release.

rhuss · 2017-01-02T15:25:02Z

src/main/java/io/fabric8/maven/docker/log/SharedPrintStream.java

+        return printStream;
+    }
+
+    public void setPrintStream(PrintStream printStream) {


Doesnt look like that this method is ever used. Also, removing this set-method makes the class immutable with respect to the printstream which is a good thing :)

rhuss · 2017-01-02T15:33:44Z

src/main/java/io/fabric8/maven/docker/access/log/LogRequestor.java

@@ -91,6 +91,8 @@ public void fetchLogs() {
            parseResponse(resp);
        } catch (IOException exp) {
            callback.error(exp.getMessage());
+        } finally {
+            callback.close();


We should ensure, that, when the callback is closed that this any call to the LogRequestors method is invalid (by throwing an exception).

Its also not so nice that the responsibilities for creating the callback (== caller of constructor of LogRequestor) and the one closing the callback (== the LogRequestor) are different objects. It would be much better when the LogRequestor has the complete control over allocation and closing of the print stream.

I suggest therefor to introduce a an LogCallback open() which allocates the stream instead of automatically allocating it in the constructor. Each method which can close a stream then would have to open it before.

rhuss · 2017-01-02T15:35:23Z

src/main/java/io/fabric8/maven/docker/access/log/LogRequestor.java

@@ -224,6 +227,7 @@ public void finish() {
                }
            }
        }
+        callback.close();


finish() is called in case of an exception but the callback.close() is also included in the finally clauses of the top level methods. This leads to closing a callback() twice which is probably not intended.

rhuss · 2017-01-02T15:56:12Z

src/test/java/io/fabric8/maven/docker/log/DefaultLogCallbackTest.java

+        callback2.log(1, ts, "line 4");
+        callback2.close();
+
+        List<String> lines = Files.readAllLines(path);


Unfortunately we can't use Java 8 and are still stuck to Java 7 for now (this might change in the future). So we need some other way to read all lines here.

This change is related to fabric8io#661 to make start / stop more symmetric - removing Java 8 methods - minor cosmetics

rhuss · 2017-01-02T16:23:05Z

@hwellmann I made some changes and would like to push it to this PR. Could you please grant me push permissions ? This is described here

rhuss · 2017-01-02T21:05:47Z

Sorry, my faul. Mispelled the target branch for the push.

rhuss · 2017-01-02T22:06:35Z

[merge]

PR merged! Thanks!

chonton and others added 2 commits December 9, 2016 16:07

update samples aggregator pom; use build-nofork in lifecycles

0a6e423

Create at most one PrintStream per log file.

f727f85

This avoids corrupted log files. Fixes fabric8io#652.

chonton and others added 5 commits December 17, 2016 12:10

Extended authentication

f6d90e7

AWS ECR key exchange closes fabric8io#637 Signed-off-by: Chas Honton <chas@apache.org>

added unit tests for extended aws ecr authentication

fd71317

Signed-off-by: Chas Honton <chas@apache.org>

Add a .codecov.yml to tune codecov

91170d4

Merge pull request fabric8io#665 from rhuss/master

ecf482e

PR merged! Thanks!

Merge pull request fabric8io#662 from chonton/fix-samples

67fa15b

PR merged! Thanks!

rhuss requested changes Dec 20, 2016

View reviewed changes

rhuss added the changes requested label Dec 20, 2016

hwellmann added 2 commits December 21, 2016 16:20

PrintStream reference counting

100b77a

Fixes fabric8io#652.

Javadoc, cleanup

82257ff

chonton and others added 15 commits December 21, 2016 09:47

more unit tests for extended authentication

e3c84af

Signed-off-by: Chas Honton <chas@apache.org>

use StringUtils to merge signing headers

4596d0f

Signed-off-by: Chas Honton <chas@apache.org>

Added volume-remove to the volume sample

0449cdc

Signed-off-by: Tom Burton <Tom.Burton@Outlook.com>

Added Volume Goals and Configuration to the documentation

873166a

Code Formatting / Documentation Changes

b1f6a87

fabric8io#664 (review) corrected spelling corrected ascii-doc + formatting code formatting Signed-off-by: Tom Burton <Tom.Burton@Outlook.com>

Code Formatting / Documentation Changes

453906b

fabric8io#664 (review) corrected spelling corrected ascii-doc + formatting code formatting Signed-off-by: Tom Burton <Tom.Burton@Outlook.com>

Update changelog do add fabric8io#664

686d5ef

Merge pull request fabric8io#669 from rhuss/master

dffb030

PR merged! Thanks!

Revert "Upgrade to jnr-unixsocket 0.15"

cee2ae0

7afec26

Merge pull request fabric8io#672 from rhuss/master

cc21086

PR merged! Thanks!

Merge branch 'master' into extended-auth

41c481b

Merge pull request fabric8io#663 from chonton/extended-auth

c77945e

PR merged! Thanks!

Some minor cosmetics from fabric8io#663

a44b821

fusesource-ci and others added 2 commits January 2, 2017 15:02

Merge pull request fabric8io#674 from rhuss/663-extended-auth

ea6c4cf

PR merged! Thanks!

Merge branch 'garbled-logs' of https://github.com/hwellmann/docker-ma…

29ab8d8

…ven-plugin into hwellmann-garbled-logs

rhuss approved these changes Jan 2, 2017

View reviewed changes

Add a LogCallback.open() for resource management.

452c0bd

This change is related to fabric8io#661 to make start / stop more symmetric - removing Java 8 methods - minor cosmetics

rhuss removed the changes requested label Jan 2, 2017

Fixed synchronization issues

b7aaed5

fusesource-ci merged commit 79e2686 into fabric8io:master Jan 2, 2017

rgbj pushed a commit to rgbj/docker-maven-plugin that referenced this pull request Jun 21, 2017

Merge pull request fabric8io#661 from hwellmann/garbled-logs

5f2af2a

PR merged! Thanks!

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create at most one PrintStream per log file. #661

Create at most one PrintStream per log file. #661

hwellmann commented Dec 17, 2016

codecov-io commented Dec 17, 2016 •

edited

Loading

rhuss left a comment

rhuss Dec 20, 2016

rhuss Dec 20, 2016

rhuss Dec 20, 2016

rhuss Dec 20, 2016

hwellmann commented Dec 20, 2016

rhuss commented Dec 20, 2016 via email

hwellmann commented Dec 21, 2016

rhuss left a comment

rhuss Jan 2, 2017

rhuss Jan 2, 2017

rhuss Jan 2, 2017

rhuss Jan 2, 2017

rhuss commented Jan 2, 2017

rhuss commented Jan 2, 2017

rhuss commented Jan 2, 2017

Create at most one PrintStream per log file. #661

Create at most one PrintStream per log file. #661

Conversation

hwellmann commented Dec 17, 2016

codecov-io commented Dec 17, 2016 • edited Loading

Current coverage is 48.96% (diff: 59.50%)

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwellmann commented Dec 20, 2016

rhuss commented Dec 20, 2016 via email

hwellmann commented Dec 21, 2016

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss commented Jan 2, 2017

rhuss commented Jan 2, 2017

rhuss commented Jan 2, 2017

codecov-io commented Dec 17, 2016 •

edited

Loading