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

Update zero-allocation-hashing and xx hash method name #959

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shivamgupta1
Copy link
Contributor

@shivamgupta1 shivamgupta1 commented Dec 20, 2023

Summary

The zero-allocation-hashing library versions > 0.7 are missing the method LongHashFunction.xx_r39 required by pegasus, thus causing failures. However, it appears that the newer versions are being increasingly used by OSS libraries and thus with LI too.

It looks like they just renamed xx_r39 (xxHash algo release 39) to xx to stop tying it to a particular release: OpenHFT/Zero-Allocation-Hashing@658079a

Thus, it should be safe for us to move over to xx.

@zackthehuman
Copy link
Contributor

I think this is going to be a breaking change, or at least require a lot of spot fixing when it re-enters the internal LinkedIn ecosystem. If you check internal code search, we use LongHashFunction.xx_r39 all over the place. I think this was a mistake on behalf of the library owner to remove LongHashFunction.xx_r39 from the public API on a minor release. Since this is a minor version bump for pegasus, it's going to result in broken builds for any code base that pulls in pegasus with this change. We are not currently planning on releasing the next major version of pegasus, so a change like this is worrisome. Can you help em understand what the rollout plan is? (We can discuss internally). cc @mchen07

Side note: I don't think we should link to our internal systems (JIRA/Slack/etc.) from OSS PRs.

@bohhyang
Copy link
Contributor

can we roll it out with a minor version bump, like 29.49.0?

@zackthehuman
Copy link
Contributor

can we roll it out with a minor version bump, like 29.49.0?

Not without introducing runtime breaking changes and a lot of toil to clean up. Instead, we should try to migrate our internal usage of LongHashFunction.xx_r39 to LongHashFunction.xx. We are trying to upstream as patch to make that possible: OpenHFT/Zero-Allocation-Hashing#84. If we can leverage that patch then we will have a smooth path forward that doesn't involve breaking pegasus users' runtimes.

@@ -104,7 +104,7 @@ project.ext.externalDependency = [
'spock': 'org.spockframework:spock-core:1.3-groovy-2.5',
'testng': 'org.testng:testng:6.13.1',
'velocity': 'org.apache.velocity:velocity-engine-core:2.2',
'zero_allocation_hashing': 'net.openhft:zero-allocation-hashing:0.7',
'zero_allocation_hashing': 'net.openhft:zero-allocation-hashing:0.16',
Copy link
Contributor

Choose a reason for hiding this comment

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

this can use the custom fork now, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not yet; the custom fork is only released internally to LinkedIn. We need to update internal users to use our fork, and then change their code to use xx() instead of xx_r39(). After that, we are free to use net.openhft:zero-allocation-hashing:0.16 in Pegasus. We won't need to fork anymore at that point.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it. Thanks!

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.

3 participants