[#8662] Fix(core): Correctly cache entities with a relationType

[keepConcentration]

What changes were proposed in this pull request?

This PR addresses an issue in CaffeineEntityCache.java where entities with a non-null relationType were not being cached as intended.
The problem was traced to a return statement in the syncEntitiesToCache method that incorrectly prevented the caching of relation-based data.
This change removes the aforementioned statement.

Why are the changes needed?

The previous behavior caused the cache to be bypassed for these operations, leading to unnecessary backend requests and preventing intended performance benefits. This fix ensures the caching mechanism now operates as designed.

Fix: #8662

Does this PR introduce any user-facing change?

No user-facing changes.

How was this patch tested?

Executed existing unit tests.
Added a new unit test (testPutAndMergeWithRelationType) to verify that the fix works as expected.

[yuqi1129]

So, is it by design that we will not cache the relation data?

[yuqi1129]

It's supposed to be a deliberate design. @xunliu can you help to verify it?

[keepConcentration]

Hi @yuqi1129, thanks for the review.

Looking at PR #8297 where this code was introduced, it seems the original intention was to cache relational data. However, the if (key.relationType() != null) return; line appears to prevent this behavior.

[yuqi1129]

Yeah, that also puzzles me.

[jerryshao]

I think this is already fixed by @xunliu in #8297, @xunliu @yuqi1129 can you please confirm this?

[yuqi1129]

Accordingly to the code, if if (key.relationType() != null) return; exists, we will not cache all entities associated with the relation, and the code looks quite weird.

gravitino/core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java

Lines 305 to 324 in 1eadcc2

	private void syncEntitiesToCache(EntityCacheRelationKey key, List<Entity> newEntities) {
	if (key.relationType() != null) return;
	List<Entity> existingEntities = cacheData.getIfPresent(key);
	if (existingEntities != null && key.relationType() != null) {
	Set<Entity> merged = Sets.newLinkedHashSet(existingEntities);
	merged.addAll(newEntities);
	newEntities = new ArrayList<>(merged);
	}
	cacheData.put(key, newEntities);
	for (Entity entity : newEntities) {
	reverseIndex.indexEntity(entity, key);
	}
	if (cacheData.policy().getIfPresentQuietly(key) != null) {
	cacheIndex.put(key.toString(), key);
	}

[yuqi1129]

I can assure you that this is specifically designed in the current code base. If we enable caching data for relational operations, something will be wrong with the current cache layer.

[yuqi1129]

@keepConcentration
It's expected that CI will fail after your change. Do you happen to have time to try to fix the failure? Just to let you know, this is not a major issue.

[keepConcentration]

@keepConcentration

It's expected that CI will fail after your change. Do you happen to have time to try to fix the failure? Just to let you know, this is not a major issue.

Thank you for letting me know. I will review the issue and provide a fix within this week.

[keepConcentration]

@keepConcentration It's expected that CI will fail after your change. Do you happen to have time to try to fix the failure? Just to let you know, this is not a major issue.

Hi, @yuqi1129
I ran the tests locally and they all pass. Could you share which specific tests are expected to fail in CI? Understanding the failure cases would help me address the root cause.

Thank you.

[yuqi1129]

@keepConcentration It's expected that CI will fail after your change. Do you happen to have time to try to fix the failure? Just to let you know, this is not a major issue.

Hi, @yuqi1129 I ran the tests locally and they all pass. Could you share which specific tests are expected to fail in CI? Understanding the failure cases would help me address the root cause.

Thank you.

Please closely watch the failure test cases in the CI.

[keepConcentration]

@keepConcentration It's expected that CI will fail after your change. Do you happen to have time to try to fix the failure? Just to let you know, this is not a major issue.

Hi, @yuqi1129 I ran the tests locally and they all pass. Could you share which specific tests are expected to fail in CI? Understanding the failure cases would help me address the root cause.
Thank you.

Please closely watch the failure test cases in the CI.

It seems that intentionally not caching relational data is a design choice. Based on PR #8297, specifically the commit fba3b80 (Disable cache multi-layer relation entity), this behavior was deliberately added.

[yuqi1129]

@keepConcentration It's expected that CI will fail after your change. Do you happen to have time to try to fix the failure? Just to let you know, this is not a major issue.

Hi, @yuqi1129 I ran the tests locally and they all pass. Could you share which specific tests are expected to fail in CI? Understanding the failure cases would help me address the root cause.
Thank you.

Please closely watch the failure test cases in the CI.

It seems that intentionally not caching relational data is a design choice. Based on PR #8297, specifically the commit fba3b80 (Disable cache multi-layer relation entity), this behavior was deliberately added.

I think so, and we can close this PR if it's hard to move on.

[keepConcentration]

@keepConcentration It's expected that CI will fail after your change. Do you happen to have time to try to fix the failure? Just to let you know, this is not a major issue.

Hi, @yuqi1129 I ran the tests locally and they all pass. Could you share which specific tests are expected to fail in CI? Understanding the failure cases would help me address the root cause.
Thank you.

Please closely watch the failure test cases in the CI.

It seems that intentionally not caching relational data is a design choice. Based on PR #8297, specifically the commit fba3b80 (Disable cache multi-layer relation entity), this behavior was deliberately added.

I think so, and we can close this PR if it's hard to move on.

Thanks for confirming.