rishabhdaim commented on code in PR #2947:
URL: https://github.com/apache/jackrabbit-oak/pull/2947#discussion_r3397266589
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/spi/toggle/Feature.java:
##########
@@ -22,8 +22,11 @@
import java.util.Collections;
import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.jackrabbit.oak.commons.properties.SystemPropertySupplier;
import org.apache.jackrabbit.oak.spi.whiteboard.Registration;
import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard;
+import org.slf4j.Logger;
Review Comment:
These `Logger` and `LoggerFactory` imports are not used anywhere in
`Feature.java`.
Oak usually keeps imports clean; leaving them in may trip checkstyle or
confuse readers. Drop both imports.
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/spi/toggle/Feature.java:
##########
@@ -60,7 +63,9 @@ private Feature(AtomicBoolean value, Registration
registration) {
* @return the feature toggle.
*/
public static Feature newFeature(String name, Whiteboard whiteboard) {
- AtomicBoolean value = new AtomicBoolean();
+ // by default the initial value is false, but it can be overridden by
a system property
+ AtomicBoolean value = new AtomicBoolean(
+ SystemPropertySupplier.create("oak-feature." + name,
false).get());
Review Comment:
`newFeature` now honors `-Doak-feature.<name>` via `SystemPropertySupplier`,
but that contract is only described in an inline comment.
`org.apache.jackrabbit.oak.spi.toggle` is exported — operators and
integrators read the Javadoc, not comments. Document the property name pattern
(`oak-feature.<name>`), that it is read once at creation, and that the default
stays `false` when unset (class-level Javadoc and on `newFeature`).
##########
oak-core-spi/src/test/resources/logback-test.xml:
##########
@@ -27,7 +27,7 @@
</encoder>
</appender>
-<root level="ERROR">
+<root level="INFO">
Review Comment:
Raising the module test root logger from `ERROR` to `INFO` affects every
test in `oak-core-spi`, not just `FeatureToggleTest`.
That is likely only needed because `SystemPropertySupplier` logs at INFO
when a property differs from default. Prefer a targeted logger instead:
```xml
<logger
name="org.apache.jackrabbit.oak.commons.properties.SystemPropertySupplier"
level="INFO"/>
```
…and keep `<root level="ERROR">`, or call `logSuccessAs("TRACE")` on the
supplier in production code.
##########
oak-core-spi/src/test/java/org/apache/jackrabbit/oak/spi/toggle/FeatureToggleTest.java:
##########
@@ -50,6 +50,17 @@ public void disabledByDefault() {
}
}
+ @Test
+ public void defaultOverridden() {
Review Comment:
`defaultOverridden` covers the happy path (`oak-feature.my.toggle=true`).
Optional: add cases for an explicit `false` override and for sysprop `true`
followed by `FeatureToggle.setEnabled(false)` on the same toggle name — the
existing `register()` test uses a different name and does not cover that
interaction.
--
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]
