-
Notifications
You must be signed in to change notification settings - Fork 232
Fix concurrent modification on logs and tags #500
Fix concurrent modification on logs and tags #500
Conversation
JaegerSpan span = tracer.buildSpan("foo").start(); | ||
span.log("foo"); | ||
for (LogData log: span.getLogs()) { | ||
span.log("foo2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test failing when ran against the current master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so
Codecov Report
@@ Coverage Diff @@
## master #500 +/- ##
===========================================
- Coverage 88.21% 88.2% -0.02%
Complexity 498 498
===========================================
Files 65 65
Lines 1859 1848 -11
Branches 242 239 -3
===========================================
- Hits 1640 1630 -10
Misses 141 141
+ Partials 78 77 -1
Continue to review full report at Codecov.
|
@@ -61,7 +63,7 @@ | |||
this.startTimeMicroseconds = startTimeMicroseconds; | |||
this.startTimeNanoTicks = startTimeNanoTicks; | |||
this.computeDurationViaNanoTicks = computeDurationViaNanoTicks; | |||
this.tags = new HashMap<String, Object>(); | |||
this.tags = new ConcurrentHashMap<String, Object>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could also remove the synchronized from the tags getter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be removed from other methods too. I will rather do it in a separate PR with more concurrent test and keep this clear. synchronized
was not related to the concurrent modifications in this case.
I am not clear why copy on write is better than normal synchronization. |
@pavolloffay I've just commented on the issue #334 with an update on my repo. |
thanks @bygui86, I will fix this anyway as it's a bug |
Thanks a lot @pavolloffay :) |
7e094ca
to
2dc6656
Compare
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
2dc6656
to
0005f5e
Compare
@yurishkuro @vprithvi I have changed PR to use only wrap the collection in getters. In the next PR I will try to get rid of synchronizing on this with a test validating better performance. |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in getters make sense, what was the change to log methods about? Just refactoring?
@@ -23,15 +23,9 @@ | |||
private final String message; | |||
private final Map<String, ?> fields; | |||
|
|||
LogData(long time, String message) { | |||
LogData(long time, Map<String, ?> fields, String message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would have preferred the message parameter to come before fields
return log(timestampMicroseconds, null, event); | ||
} | ||
|
||
private JaegerSpan log(long timestampMicroseconds, Map<String, ?> fields, String event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would have preferred the message parameter to come before fields
Just refactoring, I think the current version is cleaner - only one method for processing logs. I will update PR to change the parameter order and merge if nobody objects |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Fixes #334
Signed-off-by: Pavol Loffay ploffay@redhat.com