NETOBSERV-334 concurrent map iteration & write fix #226
Conversation
pkg/config/config.go
Outdated
| func (m GenericMap) Copy() map[string]interface{} { | ||
| result := map[string]interface{}{} | ||
|
|
||
| for k, v := range m { | ||
| result[k] = v | ||
| } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
I do a simple flat copy here for performances purpose but we can implement a deep copy if needed
There was a problem hiding this comment.
Should this function be here or in utils?
There was a problem hiding this comment.
@KalmanMeth @jpinsonneau I had the same thought .... maybe the right thing to do is open a new GenericMap.go file
There was a problem hiding this comment.
I can move this 👍
Does a new file generic_map.go in config folder is ok for both of you ? @KalmanMeth @eranra
There was a problem hiding this comment.
pkg/config/config.go
Outdated
| @@ -118,3 +118,14 @@ func ParseConfig() error { | |||
| logrus.Debugf("params = %v ", Parameters) | |||
| return nil | |||
| } | |||
|
|
|||
| // Copy will create a flat copy of GenericMap | |||
| func (m GenericMap) Copy() map[string]interface{} { | |||
There was a problem hiding this comment.
Should the input and output be both GenericMap ???
i.e. s/ map[string]interface{} / GenericMap
pkg/pipeline/write/write_loki.go
Outdated
| @@ -231,7 +231,8 @@ func (l *Loki) processRecords() { | |||
| log.Debugf("exiting writeLoki because of signal") | |||
| return | |||
| case record := <-l.in: | |||
| err := l.ProcessRecord(record) | |||
| // copy record before process to avoid alteration on parallel stages | |||
| err := l.ProcessRecord(record.Copy()) | |||
There was a problem hiding this comment.
@jpinsonneau I suggest moving the copy process to be inside the ProcessRecord function --- the rule should be that we are not allowed to change the inputs --- and we need to maybe check also in other places that we are not doing that.
There was a problem hiding this comment.
@eranra When we add fields to enrich the reporting we change the records.
There was a problem hiding this comment.
@KalmanMeth ... so should we add copy also there ??? do you want to open a PR for that?
|
Thank you all for the feedback 👍 |
This fix
fatal error: concurrent map iteration and map writewhen callingstdoutandlokistages in parallel