This repository has been archived by the owner on Jul 7, 2020. It is now read-only.
Constrain minion log bytes to about 20MB. #281
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tedpearson
suggested changes
Jun 19, 2018
| return LessBytes.toString(buf); | ||
| } catch (Exception e) { | ||
| log.warn("", e); | ||
| } | ||
| return ""; | ||
| } | ||
|
|
||
| private static void addExceedingMsg(byte[] buf, String charsetName) { | ||
| try { | ||
| byte[] lastLine = "\nCannot display all lines. Reduce lines!\n".getBytes(charsetName); |
There was a problem hiding this comment.
I recommend making the message even more explicit, including information about:
- the byte limit, and that it was exceeeded
- change "reduce lines" to something like "reduce number of lines" or "reduce requested lines"
| return LessBytes.toString(buf); | ||
| } catch (Exception e) { | ||
| log.warn("", e); | ||
| } | ||
| return ""; | ||
| } | ||
|
|
||
| private static void addExceedingMsg(byte[] buf, String charsetName) { |
There was a problem hiding this comment.
Just hardcode the UTF-8 into this method for now
| private static void addExceedingMsg(byte[] buf, String charsetName) { | ||
| try { | ||
| byte[] lastLine = "\nCannot display all lines. Reduce lines!\n".getBytes(charsetName); | ||
| for (int lastLineIndex = 0; lastLineIndex < lastLine.length; lastLineIndex++) { |
There was a problem hiding this comment.
Wouldn't System.arraycopy() be better?
| for (int lastLineIndex = 0; lastLineIndex < lastLine.length; lastLineIndex++) { | ||
| buf[logBufLimit - lastLine.length + lastLineIndex] = lastLine[lastLineIndex]; | ||
| } | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
"Exception" is too broad.
| raf.read(buf); | ||
| byte[] buf = null; | ||
| // limiting log reads below 20MB, in case of reaching heap limit and crashing minion | ||
| if(bytesRead >= logBufLimit) { |
There was a problem hiding this comment.
Each of these 4 sections could be replaced by a call to a new method you can create. the raf.seek() calls can be made before the call to the method. Something like "readFileBytesLimited(long requestedBytes)" which returns a new byte[] created in the method.
tedpearson
approved these changes
Jun 20, 2018
|
Looks good. Let's wait to merge until after the next successful hydra release. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Limiting log byte[] to size 20M. Print out warning if exceeds.