implement cache policy #14440
implement cache policy #14440
Conversation
blcksrx
force-pushed
the
hossein/cache_strategy
branch
2 times, most recently
from
18596d0 to
e315119
Compare
Signed-off-by: Hossein Torabi <[email protected]>
blcksrx
force-pushed
the
hossein/cache_strategy
branch
from
e315119 to
e2bbabf
Compare
There was a problem hiding this comment.
Thank you for the contribution @blcksrx !
Just thinking out loud but I'm wondering if this is something that requires configurability or if there is any benefit to keep the expireAfterAccess functionality instead of simply switching to expireAfterWrite without any new configs. Let's see what others say.
| old: "method org.apache.iceberg.orc.ORC.WriteBuilder org.apache.iceberg.orc.ORC.WriteBuilder::config(java.lang.String,\ | ||
| \ java.lang.String)" | ||
| justification: "Removing deprecations for 1.2.0" | ||
| "1.10.0": |
There was a problem hiding this comment.
I don't think these changes are relevant for this PR. Would you mind taking a look?
|
|
||
| private final Catalog catalog; | ||
| private final boolean caseSensitive; | ||
| private final String cachePolicy; |
There was a problem hiding this comment.
nit: cacheExpirationPolicy?
| * <li>EXPIRE_AFTER_WRITE - cache entries are evicted frequently after cache write | ||
| * </ul> | ||
| */ | ||
| public static final String CACHE_POLICY = "cache.strategy"; |
There was a problem hiding this comment.
nit: CACHE_EXPIRATION_POLICY, "cache.expiration-policy"
| */ | ||
| public static final String CACHE_POLICY = "cache.strategy"; | ||
|
|
||
| public static final List<String> CACHE_POLICY_VALUES = |
There was a problem hiding this comment.
This could be an enum
There was a problem hiding this comment.
Regarding being enum at first glance, I went for enum but I saw there is no support for Enums in the PropertyUtils. that's why I have sticked with the String
| Caffeine<TableIdentifier, Table> cacheBuilder = | ||
| Caffeine.newBuilder() | ||
| .softValues() | ||
| .removalListener(new MetadataTableInvalidatingRemovalListener()) |
There was a problem hiding this comment.
Any reason moving these out from the if below? Ticker is only relevant there. I also wouldn't add removalListener, nor executor here
| cacheBuilder = | ||
| cacheBuilder.expireAfterAccess(Duration.ofMillis(expirationIntervalMillis)); | ||
| break; | ||
| default: |
There was a problem hiding this comment.
Wouldn't be needed if policy was an enum
There was a problem hiding this comment.
well, also it needs a switch case, right?
| public void testTableWithMetadataTableName() throws Exception { | ||
| TestableCachingCatalog catalog = | ||
| TestableCachingCatalog.wrap(hadoopCatalog(), EXPIRATION_TTL, ticker); | ||
| TestableCachingCatalog.wrap( |
There was a problem hiding this comment.
Can't we have a constructor with the old signature that defaults to the default policy? We could get away not changing these tests then.
There was a problem hiding this comment.
Well, I needed to write a test case for it, thats why I have added that.
Thank you very much. well my intention was not change the existing cache strategy, thats why I made it like that. |
2 participants
Implement cachePolicy that users can select the policy between
EXPIRE_AFTER_WRITEandEXPIRE_AFTER_ACCESS#14417