draft: major refactor #90
Conversation
| return fmt.Sprintf("interrupted with status %d and action %s", e.Interruption.Status, e.Interruption.Action) | ||
| } | ||
|
|
||
| func (e *ErrInterrupted) Is(target error) bool { |
There was a problem hiding this comment.
I think this is wrong. Is is not meant to see if two errors are the same, instead check error wrapping.
There was a problem hiding this comment.
I'm playing around with errors.As, apparently that's a shortcut to compare and cast at the same time.
It requires the implementation of Is, and it is working and passing tests
Signed-off-by: Juan Pablo Tosso <[email protected]>
|
@sts could you help us fix the e2e for this branch? I'm not sure how it works |
| const defaultExpire = time.Second * 10 | ||
| const defaultEvictionInterval = time.Second * 1 | ||
|
|
||
| func Add(tx types.Transaction, ttl time.Duration) { |
There was a problem hiding this comment.
| func Add(tx types.Transaction, ttl time.Duration) { | |
| func Set(tx types.Transaction, ttl time.Duration) { |
It will override existing entries with the same ID.
There was a problem hiding this comment.
Although you are right, this is transparent, as the ID MUST always be unique, by design
| // It should only be called when the server is shutting down | ||
| // And the SPOE server is already shutdown | ||
| // Otherwise the server might crash | ||
| func Shutdown() { |
There was a problem hiding this comment.
| func Shutdown() { | |
| func RemoveAll() { |
A function called Shutdown would imply that after calling it, prevent the usage of the cache. As everything is manipulating a global variable inside this package, I would expose it with the same name as the called Function
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestCache(t *testing.T) { |
There was a problem hiding this comment.
nit: please add some newlines between the different stages
| "github.com/rs/zerolog" | ||
| ) | ||
|
|
||
| func New(level string, file string) (*zerolog.Logger, error) { |
There was a problem hiding this comment.
nit: A package just for setting up zerolog is kinda overkill. I would prefer it to only be configured inside the main
There was a problem hiding this comment.
Yes, originally that was a full logger implementation, but it ended up as a wrapper for zerolog. I'm moving it
| url.WriteString(req.Query) | ||
| } | ||
| tx.ProcessURI(url.String(), req.Method, "HTTP/"+req.Version) | ||
| if err := readHeaders(req.Headers, func(key, value string) { |
There was a problem hiding this comment.
I also love inlining error checks but with newlines its fairly hard to read. Maybe cant you give it the function directly?
| tx.ProcessLogging() | ||
| tx.Close() | ||
| // TODO does remove forces eviction? | ||
| cache.Remove(id) |
There was a problem hiding this comment.
even if it does force eviction, we maybe want that to prevent the memory filling up
| "github.com/negasus/haproxy-spoe-go/message" | ||
| ) | ||
|
|
||
| func unmarshalMessage(msg *message.Message, out interface{}) error { |
There was a problem hiding this comment.
Please no reflection inside the hotpath, especially when you can hardcode all fields
There was a problem hiding this comment.
@fionera I think the second sentence summarizes your needs. Please refrain from using the first sentence in a public place like this 🙏
|
|
||
| var requestPool = sync.Pool{ | ||
| New: func() interface{} { | ||
| return &applicationRequest{ |
There was a problem hiding this comment.
| return &applicationRequest{ | |
| return &applicationRequest{} |
|
|
||
| var responsePool = sync.Pool{ | ||
| New: func() interface{} { | ||
| return &applicationResponse{ |
There was a problem hiding this comment.
| return &applicationResponse{ | |
| return &applicationResponse{} |
| "strings" | ||
| ) | ||
|
|
||
| func readHeaders(headers string, callback func(key string, value string)) error { |
There was a problem hiding this comment.
just throw this into application.go, as its not complicated/big enough to deserve its own file
New features
TODO: