Error messages are now printed using logger.error instead of System.error

[kishore25kumar]

Using logger for printing messages make it consistent. And it also makes sure all logs go through same logging configuration (logback.xml) which has the s3 prefix in it. Using println will bypass it.

[gaul]

These messages intentionally direct to stderr as any other command-line application. If you want to redirect when run as part of another application, perhaps you should call isatty or some Java equivalent like System.console?

[kishore25kumar]

@andrewgaul sorry I couldn't fully understand it. Can you explain a bit more about it?

[kishore25kumar]

@andrewgaul Is this change ok? You can configure it through properties

[kishore25kumar]

@andrewgaul any update on this pull request

[gaul]

@kishore25kumar Please respond to my original review comment.

[kishore25kumar]

@andrewgaul I didn't find anything similar to something you have specified. The best I could found is set the System.err to a different function and write it using a logger in the function. What to set for System.err is decided by a config flag. That is the change I have made in the pull request.

[kishore25kumar]

@andrewgaul any suggestions on the pull request? I tried answering your question in previous comment

[kishore25kumar]

@andrewgaul any suggestions on this pull request?

[gaul]

You did not respond to my original review comments. I suggested looking at an isatty alternative like System.console. Let me Google that for you:

https://stackoverflow.com/questions/1403772/how-can-i-check-if-a-java-programs-input-output-streams-are-connected-to-a-term

This would allow the common case of an interactive user getting help text formatted as all other command line programs do while logging when System.err is not connected to a terminal. This is similar to how ls decides whether to color its output. For what it's worth, S3Proxy formats its help output similar to how mysqld and other daemons do.

Your approach, adding a flag to satisfy a single niche user, is not scalable from a complexity point of view. Have you instead considered redirecting output from the s3proxy executable, e.g., sed 's/^/[s3proxy] /?

Why do you have such urgency for this logging-only patch? Just put it in your private branch if you are so committed to this.

[kishore25kumar]

@andrewgaul I have updated pull request based on system.console exists or not. Sorry for the confusion as I don't have an idea about isatty. Thanks for clarifying it helped.

[gaul]

This improves over previous commits, but the first thing I tried does not log output correctly. Part of usage goes to logger and the rest goes to stderr:

$ target/s3proxy 2>&1 | cat
[s3proxy] E 04-18 19:25:46.696 main org.gaul.s3proxy.Main:251 |::] Usage: s3proxy [options...]

 --properties FILE : S3Proxy configuration (required)
 --version         : display version (default: false)

What are you trying to accomplish with this pull request? Why do you want usage to go to logger?

[kishore25kumar]

@andrewgaul

1. In a production environment, the service is running with multiple log appenders. Logging all the messages using logger will write it to attached log appenders. Example Splunk log appender.
2. All the messages that are logged from s3proxy will have the same format as they will follow logback.xml format.
3. In a production environment, deployed instances are not accessible for debugging issues. Log appenders are the solution here.

I figured out the issue. command line parser is not using the printstream directly instead it is using outputstream and wrapping this using a writer. So finally the ouputstream write function is called instead of printstream print function. I am looking into it.

[kishore25kumar]

@andrewgaul Fixed the issue

[gaul]

Instead of bringing in some unnecessary dependency, can't you just override write? Or fix this upstream in args4j?

[kishore25kumar]

@andrewgaul we can write but I should take care of all the scenarios. If I have to write something, it will be equivalent to that one at the end. Can I copy that file into s3proxy repo instead of adding the dependency? Is that ok?

[gaul]

Sorry, I will not accept cut and paste code. Also you need to at least open an issue against args4j if you do not provide a fix.

[gaul]

Upstream issue kohsuke/args4j#149 and pull request kohsuke/args4j#150.