namespace didn't remove experiments from segmentAllocations after invoking RemoveExperiment() #4
Conversation
Hitting a nil value previously would cause a crash in the experiment run. This happens in both the case of the value being in a map, and in the case where the value is part of a struct. This fix causes the code to instead return a nil. Unfortunately this is not quite ideal in the case of maps to numerical values. In a normal go map, maps with numerical values apparently return a 0 when the requested key is not present. There doesn't seem to be any simple way to maintain parity with that behavior (though it does seem kind of odd, looking at it from Java colored glasses), so a concession is made to just return a nil in that case.
namespace.go
Outdated
| @@ -94,6 +94,10 @@ func (n *SimpleNamespace) RemoveExperiment(name string) error { | |||
| } | |||
| } | |||
|
|
|||
| for _, val := range segmentsToFree { | |||
| delete(n.segmentAllocations, uint64(val)) | |||
There was a problem hiding this comment.
Could delete be done inside the loop where values are added tosegmentsToFree? (instead of iterating segmentsToFree outside separately)
https://github.com/biased-unit/planout-golang/blob/master/namespace.go#L98
|
Thanks @panjf2000 for this change. Could you please add unit tests? |
|
@tsujeeth sure, i will add unit tests later. |
|
@tsujeeth already added unit tests, please review that. |
|
@panjf2000 sorry it has taken me a while to get back. Some comments:
|
namespace didn't remove experiments from segmentAllocations after invoking RemoveExperiment() and it caused "panic: runtime error: invalid memory address or nil pointer dereference" in
