Skip to content

Conversation

@leonfin
Copy link
Contributor

@leonfin leonfin commented Jan 10, 2025

  • GEODE-10453 - in case of REMOVE_DUE_TO_GII_TOMBSTONE_CLEANUP and CompactRangeIndex, specify not to lookup old key, which is very expensive operation. It's actually broken and regression. All the tombstone entries are going to be NullToken and cause class cast exception for every single remove compare if looking up old key. There is no old key during initial tombstone image sync up from lead peer.

For all changes:

  • [ GEODE-10453] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • [Yes ] Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • [ Yes] Is your initial contribution a single, squashed commit?

  • [ Yes] Does gradlew build run cleanly?

  • [ No] Have you written or updated unit tests to verify your changes?

  • [ No] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

…actRangeIndex, specify not to lookup old key, which is very expensive operation. It's actually broken and regression. All the tombstone entries are going to be NullToken and cause class cast exception for every single remove compare if looking up old key. There is no old key during initial tombstone image sync up from lead peer.
@wmh1108-sas
Copy link
Contributor

@JinwooHwang-SAS can you try kicking off the build for this one again? The contributor has requested this be merged and we need to be sure the tests are passing.

@JinwooHwang
Copy link
Contributor

@leonfin, Would you mind re-running the failed checks when you have a chance? I’m sorry for the inconvenience — it seems I don’t have permission to restart them myself.

@JinwooHwang JinwooHwang reopened this Aug 27, 2025
@JinwooHwang
Copy link
Contributor

Restarted the checks.

@leonfin
Copy link
Contributor Author

leonfin commented Aug 27, 2025

@JinwooHwang-SAS

Just for my information, how do you restart the checks? I don't see any option for that under Checks tab on PR.

It seems something wrong with docker compose setup? One of acceptance checks failed with
`> Task :geode-assembly:acceptanceTest

DualServerSNIAcceptanceTest > initializationError FAILED
org.testcontainers.containers.ContainerLaunchException: Local Docker Compose not found. Is docker-compose on the PATH?`

Who can fix it?

Thank you!

@JinwooHwang
Copy link
Contributor

@leonfin, I had to reopen it to trigger the checks. The errors in :geode-assembly:acceptanceTest have been addressed. We plan to merge this as soon as the rest of checks pass. Thanks.

@leonfin
Copy link
Contributor Author

leonfin commented Aug 27, 2025

BTW original author even said in the original commit
https://bitbucket.tools.tsimagine.com/projects/CORE/repos/geode/commits/9e7db8602df299af57d2ea824ba656fd24d54600#geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
"* Optimized index removal to not have to find old key when cleaning up gii tombstones"

Not sure why down the code path it was missed that for GII (get initial image), the findOldKey was defaulted to true. We're already using this patched version in prod for 6+ months.

@JinwooHwang JinwooHwang self-requested a review August 27, 2025 20:34
@JinwooHwang JinwooHwang merged commit d834e94 into apache:develop Aug 27, 2025
29 of 33 checks passed
JinwooHwang pushed a commit that referenced this pull request Sep 5, 2025
…actRangeIndex, specify not to lookup old key, which is very expensive operation. It's actually broken and regression. All the tombstone entries are going to be NullToken and cause class cast exception for every single remove compare if looking up old key. There is no old key during initial tombstone image sync up from lead peer. (#7890)

Co-authored-by: Leon Finker <[email protected]>
(cherry picked from commit d834e94)
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