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

JBTM-3883 use a base64 encoder if required #2281

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Conversation

mmusgrov
Copy link
Member

@mmusgrov mmusgrov commented Jul 24, 2024

https://issues.redhat.com/browse/JBTM-3883

I re-opened JBTM-3883 to add this extra check. I added a comment to the issue and to the test in this PR that explains the details of why the base64 encoder might be needed.

CORE !AS_TESTS !RTS !JACOCO !XTS !QA_JTA !QA_JTS_OPENJDKORB !PERFORMANCE !LRA !DB_TESTS !mysql !db2 !postgres !oracle

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@mmusgrov mmusgrov removed the request for review from marcosgopen July 25, 2024 02:18
@mmusgrov mmusgrov added the Hold label Jul 25, 2024
@mmusgrov
Copy link
Member Author

I've put this on hold while I think through the impact on crash recovery..

@mmusgrov
Copy link
Member Author

mmusgrov commented Jul 30, 2024

I've removed the Hold label because Marco, or Manuel, correctly pointed out to me that SHA-224 hashing and base64 encoders are deterministic functions and as such will always produce the same output for a given input string, ie assuming that any particular integration gives us the same string we'll be able to detect records that "belong" to the recovery manager.

Note that hashing and slicing, although increasing the probability of collisions, are acceptable because the chances of that happening are extremely low (there was some analysis about probabilities in the associated quarkus issue and PR.

@mmusgrov mmusgrov removed the Hold label Jul 30, 2024
Copy link
Member

@marcosgopen marcosgopen left a comment

Choose a reason for hiding this comment

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

Thanks Mike for your PR, LGTM!

@mmusgrov mmusgrov merged commit 33e07e4 into jbosstm:main Jul 30, 2024
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