nkuprins commented on issue #989:
URL: https://github.com/apache/fesod/issues/989#issuecomment-5280962469
Hi @bengbengbalabalabeng , thank you for taking the time to address this
issue!! I analysed your proposal carefully, and here is my opinion:
On **1.** Completely agree.
On **2.** Completely agree.
On **3.** I like your idea with strategies a lot, but I have a few questions
and an updated variant:
**3.1**
What is the plan for the public static fields in `ClassUtils`, such as
`FIELD_CACHE`? With `new MetadataCacheStrategy.MemoryCache<>()` the strategy
creates its own map, so it's no longer the same object those fields point at.
My guess is that's what the `MemoryCache(Map<K, V> cache)` overload is for,
i.e.:
```java
map.put(CacheLocationEnum.MEMORY, new
MetadataCacheStrategy.MemoryCache<>(ClassUtils.FIELD_CACHE));
```
so `ClassUtils` keeps the fields exactly as they are today and the strategy
just caches into them.
**3.2** On `clearCache(boolean clearAll)` with `true` argument.
`CacheLocationEnum.MEMORY` is documented as *"The cache will not be cleared
unless the app is stopped."*. The only caller that would pass `true` is
`ClassUtilsTest`, which currently resets state via the public fields. So the
question is whether this parameter is even needed? I guess not.
**3.3**
Here are a few small variations I'd suggest:
**3.3.1**
build the `EnumMap` once inside a per-tier holder rather than in each
resolver's static block. As otherwise `SheetHeadFieldResolver` and
`SheetContentPropertyResolver` need three between them - the content side has
two tiers, CONTENT_CACHE and CLASS_CONTENT_CACHE.
**3.3.2**
Another thing is that the current `default:` throws
`UnsupportedOperationException("unsupported enum")`. I think we can keep it, as
I do in `at(CacheLocationEnum cacheLocation)` below.
**3.3.3**
This is the updated variant with your `MetadataCacheStrategy` unchanged:
```java
/** The caches for one kind of metadata, one per CacheLocationEnum constant.
*/
final class MetadataCaches<K, V> {
private final Map<CacheLocationEnum, MetadataCacheStrategy<K, V>>
byLocation;
MetadataCaches(Map<K, V> memoryCache) {
Map<CacheLocationEnum, MetadataCacheStrategy<K, V>> map = new
EnumMap<>(CacheLocationEnum.class);
map.put(CacheLocationEnum.MEMORY, new
MetadataCacheStrategy.MemoryCache<>(memoryCache));
map.put(CacheLocationEnum.THREAD_LOCAL, new
MetadataCacheStrategy.ThreadLocalCache<>());
map.put(CacheLocationEnum.NONE,
MetadataCacheStrategy.NoOpCache.instance());
this.byLocation = Collections.unmodifiableMap(map);
}
MetadataCacheStrategy<K, V> at(CacheLocationEnum cacheLocation) {
MetadataCacheStrategy<K, V> strategy = byLocation.get(cacheLocation);
if (strategy == null) {
throw new UnsupportedOperationException("unsupported enum");
}
return strategy;
}
void removeThreadLocal() {
at(CacheLocationEnum.THREAD_LOCAL).clear();
}
}
```
```java
final class SheetHeadFieldResolver {
private static final MetadataCaches<FieldCacheKey, FieldCache> FIELD =
new MetadataCaches<>(ClassUtils.FIELD_CACHE);
static FieldCache resolve(Class<?> clazz, ConfigurationHolder
configurationHolder) {
return FIELD
.at(configurationHolder.globalConfiguration().getFiledCacheLocation())
.get(new FieldCacheKey(clazz, configurationHolder), key ->
doResolve(clazz, configurationHolder));
}
static void removeThreadLocalCache() {
FIELD.removeThreadLocal();
}
}
```
```java
final class SheetContentPropertyResolver {
private static final MetadataCaches<ContentPropertyKey,
ExcelContentProperty> CONTENT = new MetadataCaches<>(ClassUtils.CONTENT_CACHE);
private static final MetadataCaches<Class<?>, Map<String,
ExcelContentProperty>> CLASS_CONTENT = new
MetadataCaches<>(ClassUtils.CLASS_CONTENT_CACHE);
private static Map<String, ExcelContentProperty>
declaredFieldContentMap(Class<?> clazz, ConfigurationHolder
configurationHolder) {
if (clazz == null) {
return null;
}
return CLASS_CONTENT
.at(configurationHolder.globalConfiguration().getFiledCacheLocation())
.get(clazz, key -> doDeclaredFieldContentMap(clazz));
}
static void removeThreadLocalCache() {
CONTENT.removeThreadLocal();
CLASS_CONTENT.removeThreadLocal();
}
}
```
What do you think about this? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
