Merged
Conversation
`Topology.invalid` was an array using the `LogLineNumFields` constant and so was tightly coupled with the current definition of `LogLine`. Convert it to a map protected with a RWMutex (previously the array elements were atomically incremented).
tommyblue
approved these changes
Oct 27, 2020
|
|
||
| If the hardcoded values for `LogLineNumFields` and `NumFieldsBaker` do not suit | ||
| your needs, it's advised that you copy `logline.go` in your project and modify | ||
| the constants declared at the top of the file. Your specialized `LogLine` will |
Contributor
There was a problem hiding this comment.
the user should copy the logline but also rename the object, if I'm not mistaken. If true, I'd also change the code example below using something like MyCustomLogline
Collaborator
Author
There was a problem hiding this comment.
No it's not necessary since the user will do this in its own project so in a different package so the definitions are never going to clash
| if ok, idx := t.validate(record); !ok { | ||
| atomic.AddInt64(&t.invalid[idx], 1) | ||
| ok, idx := t.validate(record) | ||
| if !ok { |
Contributor
There was a problem hiding this comment.
is it as fast as the previous version?
Collaborator
Author
There was a problem hiding this comment.
No it's not since incrementing an atomic is faster than locking a mutex. However this is the only correct way to do it. Moreover the change is only seen when there's an invalid logline, the happy path stays the same
Co-authored-by: Tommaso Visconti <tommaso.visconti@adroll.com>
Co-authored-by: Tommaso Visconti <tommaso.visconti@adroll.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
❓ What
Make
baker.LogLinemore easily customizable.LogLine hardcoded constants are now only used in
logline.goso that if one copies that file to their project all continues to work.Document how to do it in
README.mdDescribe what this pull request does.
🔨 How to test
✅ Checklists
This section contains a list of checklists for common uses, please delete the checklists that are useless for your current use case (or add another checklist if your use case isn't covered yet).
make gofmt-writebeen run on the code?make govetbeen run on the code? Has the code been fixed accordingly to the output?