Fix notify silently fails #146
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
Contributor
parrasajad
commented
Jun 10, 2022
parrasajad
commented
- set buffer to file size
- split line into multiple chunks if line size is greater than char-limit
…reater than char-limit
Mzack9999
requested changes
Jun 10, 2022
|
Kudos, SonarCloud Quality Gate passed!
|
Mzack9999
approved these changes
Jun 10, 2022
ehsandeep
approved these changes
Jun 10, 2022
|
@ehsandeep I'm having problems with this, I think it needs to be backed out. Before this PR was landed: File-based and stdin with bulk and non-bulk all work. As of this PR being landed: stdin is broken, and file-based bulk is broken I can see that this PR is getting the size of the infile. I suspect this breaks for stdin, and for file-based it could cause memory pressure given a large infile. I think we need to write a new |
|
@justinsteven thanks for pointing it out, it should be addressed in #149 |
Closed
justinsteven
added a commit
to justinsteven/notify
that referenced
this pull request
Jun 12, 2022
This reverts commit 173f914.
ehsandeep
pushed a commit
that referenced
this pull request
Jun 13, 2022
* Revert "Fix notify silently fails (#146)" This reverts commit 173f914. * Emit logged lines to stdout, not stderr Fixes #134 (this was previously fixed but got backed out in landing #130) * Split lines as per CharLimit in non-bulk mode Resolves #137 * Bubble up any Scanner errors This surfaces the error that's causing #138: 'bufio.Scanner: token too long' * Fix infinite loop with -char-limit <= len('...') We're using '...' to indicate that a line has been truncated. If -char-limit was less than the length of this ellipsis string, the scanner would never terminate. Raise an error if the charLimit given to a splitter is <= len('...') * Ensure we never allow the Scanner buffer to fill We know we never need more than CharLimit chars from the Scanner in one go First of all, we ensure that the Scanner has a buffer which can hold at least CharLimit chars. Then we handle cases where the Scanner wants more data but we don't need it to get more data. Thus it should never end up in a place where its internal buffer is filled, and it should never return bufio.ErrToLong Fixes #138 * fmt.print => gologger Co-authored-by: mzack <marco.rivoli.nvh@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.