config: export & list defaults #52
Conversation
mreiferson
changed the title
|
I like it, and I think it'll work... just a few comments |
| @@ -16,112 +16,87 @@ import ( | |||
|
|
|||
| // Config is a struct of NSQ options | |||
| // | |||
| // (see Config.Set() for available parameters) | |||
| // Use Config.Set(key string, value) as an alternate way to set parameters | |||
| type Config struct { | |||
| sync.RWMutex | |||
There was a problem hiding this comment.
we probably don't need this lock anymore
There was a problem hiding this comment.
Is it important for Set() itself to be goroutine safe? The uses in consumer can easily be switched to the consumer mutex.
There was a problem hiding this comment.
I don't think so... I had only added this mutex so that if (certain) values were changed the producer's goroutines could read them safely, which is no longer necessary since we copy values in at instantiation time.
| MaxInFlight int `opt:"max_in_flight" min:"0" default:"1"` | ||
|
|
||
| // Maximum amount of time to backoff when processing fails 0 == no backoff | ||
| MaxBackoffDuration time.Duration `opt:"max_backoff_duration" min:"0" max:"60m" default:2m` |
There was a problem hiding this comment.
test is failing because this default value needs to be a string - so it's not actually being set in setDefaults
|
@mreiferson ready for final review pass. |
| r.config.RLock() | ||
| mif := r.config.maxInFlight | ||
| r.config.RUnlock() | ||
| r.mtx.RLock() |
There was a problem hiding this comment.
I don't think we can use the existing mutex to cover this value (I'm pretty sure this was intentional, see #29 etc.).
Do you want to just make those few small "adjust max in flight interface" renames/doc updates here? We could switch the Consumer owned variable for current max in flight to an atomically updated/read variable (no locks, fewer LOC)
* export config variables so they can be set directly (restore type safety) * move defaults to struct tags (and considate docs) * add Config.Valdate() and panic when NewConfig() is not used * copy Config into Consumer and Producer so it's immutable * rename SetMaxInFlight to ChangeMaxInFlight to more appropriately describe functionality
|
nice work 👍 |
|
💣s away |
config: export & list defaults
|
This looks like a nice solution.
That's awesome! 👍 |
@mreiferson this is a continuation of conversation in #47
This makes all config values exported, but copies them into a separate config object in Consumer and Producer so they can only be modified before you pass to a Consumer and Producer. This should mitigate the previous need to unexport fields.
I've also added a new way to denote defaults by marking them with struct tags. I like how this comes out by avoiding comments that might stray from the code values actually defaulting. I took a slightly different approach to just panic if an uninitialized config object is ever encountered.
thoughts?