jsedding commented on code in PR #2819: URL: https://github.com/apache/jackrabbit-oak/pull/2819#discussion_r3021979750
########## oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/impl/lirs/package-info.java: ########## @@ -0,0 +1,26 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * <em>For Oak internal use only. Do not use outside Oak components.</em> + */ +@Internal(since = "1.0.0") +@Version("1.0.0") +package org.apache.jackrabbit.oak.cache.api.impl.lirs; + +import org.apache.jackrabbit.oak.commons.annotations.Internal; +import org.osgi.annotation.versioning.Version; Review Comment: We should not export this package. bnd defaults to exporting packages that define a package-info.java file with @Version annotation. Maybe that's not the case here, but I would remove this file regardless. ########## oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/impl/caffeine/package-info.java: ########## @@ -0,0 +1,26 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * <em>For Oak internal use only. Do not use outside Oak components.</em> + */ +@Internal(since = "1.0.0") +@Version("1.0.0") +package org.apache.jackrabbit.oak.cache.api.impl.caffeine; + +import org.apache.jackrabbit.oak.commons.annotations.Internal; +import org.osgi.annotation.versioning.Version; Review Comment: We should not export this package. bnd defaults to exporting packages that define a package-info.java file with @Version annotation. Maybe that's not the case here, but I would remove this file regardless. ########## oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/impl/package-info.java: ########## @@ -0,0 +1,26 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * <em>For Oak internal use only. Do not use outside Oak components.</em> + */ +@Internal(since = "1.0.0") +@Version("1.0.0") +package org.apache.jackrabbit.oak.cache.api.impl; Review Comment: The impl package should be next to the api package, not inside. ########## oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/CacheStats.java: ########## @@ -32,7 +32,7 @@ * @param totalLoadTime total time spent loading new values, in nanoseconds * @param evictionCount number of entries evicted from the cache */ -public record OakCacheStats( +public record CacheStats( Review Comment: I would rename this class to `CacheStatsSnapshot`. That way we can reduce the confusion with rgd. to the existing `org.apache.jackrabbit.oak.cache.CacheStats` class, which provides a live-view of the stats. ########## oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/impl/CacheBuilder.java: ########## @@ -14,20 +14,28 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.jackrabbit.oak.cache; +package org.apache.jackrabbit.oak.cache.api.impl; Review Comment: I would say the `CacheBuilder` should be in the `api` package. ########## oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/impl/CacheImplementation.java: ########## @@ -0,0 +1,34 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.cache; + +/** + * Selects the backing cache implementation used by {@link OakCacheBuilder}. + * + * <p>Pass to {@link OakCacheBuilder#implementation(CacheImplementation)} to pin a specific + * cache to one backend, overriding the global {@code oak.cache.type} system property. + * When no per-instance override is set, the builder resolves the implementation from + * {@code System.getProperty("oak.cache.type", "lirs")}.</p> + */ +public enum CacheImplementation { + + /** LIRS (Low Inter-reference Recency Set) eviction, backed by {@code CacheLIRS}. */ + LIRS, Review Comment: Ok, but can we this out of the `CacheBuilder`? I would rather add `CacheLIRS#asOakCache()` to the `CacheLIRS` class. That method would simply wrap `this` in `LirsLoadingCacheAdapter`. The class `LirsLoadingCacheAdapter` could become a package-visible class next to `CacheLIRS`. In the next step, that should allow usages of the cache to be adjusted to the new APIs, while `CacheLIRS` is still in use. We can then implement a toggle to switch `CacheLIRS` to `Caffeine`. IMHO that allows us to keep the new caching API clean(er). We don't have to use the new builder for legacy cache initialization. Does that make sense? ########## oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/impl/package-info.java: ########## @@ -0,0 +1,26 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * <em>For Oak internal use only. Do not use outside Oak components.</em> + */ +@Internal(since = "1.0.0") +@Version("1.0.0") +package org.apache.jackrabbit.oak.cache.api.impl; + +import org.apache.jackrabbit.oak.commons.annotations.Internal; +import org.osgi.annotation.versioning.Version; Review Comment: We should not export this package. bnd defaults to exporting packages that define a package-info.java file with @Version annotation. Maybe that's not the case here, but I would remove this file regardless. ########## oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/impl/CacheStatsAdapter.java: ########## @@ -49,21 +52,21 @@ class OakCacheStatsAdapter extends AbstractCacheStats { * @param maxWeight configured maximum weight for the cache; {@code -1} if unbounded */ @SuppressWarnings("unchecked") - OakCacheStatsAdapter( - @NotNull OakCache<?, ?> cache, + public CacheStatsAdapter( + @NotNull Cache<?, ?> cache, @NotNull String name, - @Nullable OakWeigher<?, ?> weigher, + @Nullable Weigher<?, ?> weigher, long maxWeight) { super(name); - this.cache = (OakCache<Object, Object>) cache; - this.weigher = (OakWeigher<Object, Object>) weigher; + this.cache = (Cache<Object, Object>) cache; + this.weigher = (Weigher<Object, Object>) weigher; Review Comment: I would like to see that the generics enforce that `Cache` and `Weigher` are compatible. I.e. a signature like `public <K, V> CacheStatsAdapter(Cache<K, V> cache, String name, Weigher<K, V> weigher, long maxWeight)`. I would not add generics on the class level, because `CacheStats` doesn't need them. I would add them just for the constructor, in order to get additional validation that the arguments are compatible. -- 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]
