add logstash DAO #29
add logstash DAO #29
Conversation
|
@jakub-bochenski I opened this after discussion on #22 because the elasticsearch plugin option was forcing an http protocol message rather than TCP, which is supported in this PR (as well as the items from #28). Let me know what you think! I am using this build successfully on my Jenkins. |
|
If we add separate Logstash backend we should merge #21 too |
Signed-off-by: Ted Bao <[email protected]>
Signed-off-by: Ted Bao <[email protected]>
Signed-off-by: Haikuan Bao <[email protected]>
|
Rebased the branch to make if FF to master |
jakub-bochenski
left a comment
jakub-bochenski
left a comment
There was a problem hiding this comment.
Please add some unit tests to cover the newly added paths
Please update the README with the new backend type
| BufferedReader inFromServer = new BufferedReader(new InputStreamReader(logstashClientSocket.getInputStream())); | ||
| outToServer.writeBytes(data); | ||
| logstashClientSocket.setSoTimeout(10000); | ||
| logstashClientSocket.close(); |
There was a problem hiding this comment.
resources should be closed in finally block, or even better used in try-with-resource block
There was a problem hiding this comment.
Is it a good idea to close the Socket before closing the outputstream?
Closing the outputtstream also closes the socket.
| StringWriter sw = new StringWriter(); | ||
| PrintWriter pw = new PrintWriter(sw); | ||
| exc.printStackTrace(pw); | ||
| throw new IOException(sw.toString()); |
| outToServer.close(); | ||
| inFromServer.close(); | ||
| } | ||
| catch (Exception exc) |
There was a problem hiding this comment.
can you please change this to a multi-catch with specific Exception?
|
It would be ideal if this also supported Logstash HTTP input. |
| { | ||
| logstashClientSocket = new Socket(logstashHost, logstashPort); | ||
| DataOutputStream outToServer = new DataOutputStream(logstashClientSocket.getOutputStream()); | ||
| BufferedReader inFromServer = new BufferedReader(new InputStreamReader(logstashClientSocket.getInputStream())); |
There was a problem hiding this comment.
Why opening a reader when you never read data?
|
|
||
| final String logstashHost; | ||
| final int logstashPort; | ||
| Socket logstashClientSocket; |
There was a problem hiding this comment.
Why is this not private? And why an instance variable?
| import com.cloudbees.syslog.Facility; | ||
| import com.cloudbees.syslog.MessageFormat; | ||
| import com.cloudbees.syslog.Severity; | ||
| import com.cloudbees.syslog.sender.UdpSyslogMessageSender; |
There was a problem hiding this comment.
These imports from com.cloudbees are not used
| logstashClientSocket = new Socket(logstashHost, logstashPort); | ||
| DataOutputStream outToServer = new DataOutputStream(logstashClientSocket.getOutputStream()); | ||
| BufferedReader inFromServer = new BufferedReader(new InputStreamReader(logstashClientSocket.getInputStream())); | ||
| outToServer.writeBytes(data); |
There was a problem hiding this comment.
The javadoc for this method says:
Each character in the string is written out, in sequence, by discarding its high eight bits.
Is this really a good idea? What if we have unicode characters (very likely to happen)?
|
Superseded by #59 |
4 participants
No description provided.