-
Notifications
You must be signed in to change notification settings - Fork 435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ddtrace/tracer: simplify channels #498
Conversation
This change slightly simplifies concurrency, moves some test methods to the test files, improves naming to be more consistent and adds more documentation.
76f6b78
to
f0ef7b3
Compare
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.
Looks good. The Chan
suffix is better than what was there originally.
It'll be nice when flushChan
can be refactored out in the future.
@@ -289,7 +292,7 @@ func (t *tracer) Extract(carrier interface{}) (ddtrace.SpanContext, error) { | |||
} | |||
|
|||
// flush will push any currently buffered traces to the server. | |||
func (t *tracer) flush() { | |||
func (t *tracer) flushPayload() { |
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 comment should be updated to flushPayload
.
During a previous change released as part of v1.20.0 in PR #498, the flush channel was accidentally made blocking. This would cause a deadlock on occasion when flushing after the payload was full. Tests failed to catch this problem because they were not using the constructor and instead had their own, which was made correctly. This change also addresses that problem and removes the constructor from the tests, now correctly reproducing the problem.
During a previous change released as part of v1.20.0 in PR #498, the flush channel was accidentally made blocking. This would cause a deadlock on occasion when flushing after the payload was full. Tests failed to catch this problem because they were not using the constructor and instead had their own, which was made correctly. This change also addresses that problem and removes the constructor from the tests, now correctly reproducing the problem.
During a previous change released as part of v1.20.0 in PR #498, the flush channel was accidentally made blocking. This would cause a deadlock on occasion when flushing after the payload was full. Tests failed to catch this problem because they were not using the constructor and instead had their own, which was made correctly. This change also addresses that problem and removes the constructor from the tests, now correctly reproducing the problem.
During a previous change released as part of v1.20.0 in PR #498, the flush channel was accidentally made blocking. This would cause a deadlock on occasion when flushing after the payload size became large. Tests failed to catch this problem because they were not using the constructor and instead had their own incorrect constructor. This change also addresses that problem so that tests would now correctly catch the issue.
This change slightly simplifies concurrency, moves some test methods to the test files, improves naming to be more consistent and adds more documentation.
During a previous change released as part of v1.20.0 in PR DataDog#498, the flush channel was accidentally made blocking. This would cause a deadlock on occasion when flushing after the payload size became large. Tests failed to catch this problem because they were not using the constructor and instead had their own incorrect constructor. This change also addresses that problem so that tests would now correctly catch the issue.
This change slightly simplifies concurrency, moves some test methods to
the test files, improves naming to be more consistent and adds more
documentation.
Eventually, what is now
flushChan
can also go away, but it's a bigger pieceof work that requires a lot of test rewriting.