wilfred-s commented on a change in pull request #96: [YUNIKORN-72] data race in
unit test: TestTaskStateTransitions
URL:
https://github.com/apache/incubator-yunikorn-k8shim/pull/96#discussion_r404585651
##########
File path: pkg/conf/schedulerconf.go
##########
@@ -45,7 +47,7 @@ const (
DefaultKubeBurst = 1000
)
-var configuration *SchedulerConf
+var pointer unsafe.Pointer
Review comment:
I do not see any reason why we should use unsafe. Unsafe has its uses but I
think this is not one of them.
Unsafe is not stable and can change in any go release.
The other problem is with the `Set`. `Init` will always be called before we
set a new one. If we hold on to the previous pointer that we got via a
`GetSchedulerConf()` then that could cause issues.
Atomic also does not provide any ordering guarantee, it just provides the
correct value at that point in time.
`Set` is only called in tests. Instead of replacing pointer we should update
the content of the structure created in `init`. That means the pointer will
never change. I would rather go to a full get/set structure for the config.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
