Skip to content
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

Add mutex to guard context tags #5022

Closed
wants to merge 1 commit into from

Conversation

ypatia
Copy link
Member

@ypatia ypatia commented May 29, 2024

For C-API tracing, we ended up using tags instead of config to store trace context for Jaeger traces, because we needed that to be mutable (to be able to update the tag when we'd like traces to nest). However, tags were not protected via some locking mechanism, so this PR is adding a mutex to guard tags_ variable.


TYPE: NO_HISTORY
DESC: Add mutex to guard context tags

Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This facility has been moved entirely to class RestClient in #4994. I am currently working out the build problems there (the C++ code itself is done).

In addition, mutex protection needs to happen on read in addition to write. That's not currently possible with the way this facility is implemented with StorageManager in the middle.

@ypatia
Copy link
Member Author

ypatia commented May 30, 2024

This facility has been moved entirely to class RestClient in #4994. I am currently working out the build problems there (the C++ code itself is done).

In addition, mutex protection needs to happen on read in addition to write. That's not currently possible with the way this facility is implemented with StorageManager in the middle.

Thank you @eric-hughes-tiledb for the heads up on those parallel changes. I checked your PR and added a comment.

@KiterLuc
Copy link
Contributor

This will get closed once #4994 gets merged.

@KiterLuc KiterLuc closed this Jul 18, 2024
@KiterLuc
Copy link
Contributor

After the REST refactor in #4994 this might be irrelevant or needs to be reimplemented. Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants