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

[Streaming]relink grpc/absl for streaming.so #20136

Merged
merged 1 commit into from Nov 9, 2021

Conversation

ashione
Copy link
Member

@ashione ashione commented Nov 8, 2021

To avoid exporting thrirdparty library symbol globally, these absl/grpc libs have been applied in _streaming.so.
Side-effect:
Static variables might be uninitialized if core worker lib and streaming lib both use them.

Why are these changes needed?

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@jovany-wang
Copy link
Contributor

Static variables might be uninitialized if core worker lib and streaming lib both use them.

What's the case that both streaming.so and coreworker.so use the same static var? Both of streaming lib and coreworker lib introduce it in one thirdpary?

@ashione
Copy link
Member Author

ashione commented Nov 8, 2021

Static variables might be uninitialized if core worker lib and streaming lib both use them.

What's the case that both streaming.so and coreworker.so use the same static var? Both of streaming lib and coreworker lib introduce it in one thirdpary?

In our internal production version, I try to bind ray streaming so library with ray_common deps, but two different statatic variables in _streaming.so and core_worker.so. It cannot find correct value within same symbol if core worker enable stats metric module. For example:

grpc.enable_xxx() # Set in coreworker lib
grpc.get_xxx() == True # Get in coreworker lib

# But it's disabled in other static linked library.
grpc.get_xxx() == False

@jovany-wang

@jovany-wang
Copy link
Contributor

Should we load coreworker dynamically in streaming?

@ashione
Copy link
Member Author

ashione commented Nov 8, 2021

Should we load coreworker dynamically in streaming?

Althought we load core worker library dynamically, no exported symbol makes streaming lib unaccessable.

@jovany-wang
Copy link
Contributor

Isee.

Copy link
Contributor

@jovany-wang jovany-wang left a comment

Choose a reason for hiding this comment

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

Windows failures are unrelated, merging this.

@jovany-wang jovany-wang merged commit 97259e3 into master Nov 9, 2021
@jovany-wang jovany-wang deleted the fix-exporterd-grpc-symbol branch November 9, 2021 06:13
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.

None yet

4 participants