Support incubator features preview in zilla release docker image #753
Conversation
|
|
||
| @Override | ||
| public String name() | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This can be removed, agree?
| <version>1.1.0.7</version> | ||
| <license>The Apache License, Version 2.0</license> | ||
| </artifact> | ||
| </license-lookup> |
There was a problem hiding this comment.
We don't need this file, right?
There was a problem hiding this comment.
I was half through and hasn't done cleanup just wanted to get your initial feedback with design first.
| } | ||
|
|
||
| return construct.apply(unmodifiableMap(factoriesByName)); | ||
| } |
There was a problem hiding this comment.
Given that some services (like commands) don't have names to store in a map, I think this instantiate method goes back to FactorySpi, but without the check for incubator.
Instead, I think we want to simplify to create a filtered Iterable<S> since that's at the core of what we're trying to do with @Incubating annotation.
package io.aklivity.zilla.runtime.common.feature;
public final class FeatureLoader
{
public static final boolean INCUBATOR_ENABLED = incubatorEnabled();
public static final Predicate<Object> FEATURE_ENABLED = FeatureLoader::featureEnabled;
public static <S> Iterable<S> load(Class<S> service)
{
Iterable<S> loader = ServiceLoader.load(service);
return StreamSupport.stream(loader.spliterator(), false)
.filter(FEATURE_ENABLED)::iterator;
}
private static boolean featureEnabled(
Object feature)
{
return INCUBATOR_ENABLED || !feature.getClass().isAnnotationPresent(Incubating.class);
}
private static boolean incubatorEnabled()
{
final Module module = Feature.class.getModule();
final String override = System.getProperty("zilla.incubator.enabled");
return override != null
? Boolean.parseBoolean(override)
: module == null ||
"develop-SNAPSHOT".equals(module
.getDescriptor()
.version()
.map(ModuleDescriptor.Version::toString)
.orElse("develop-SNAPSHOT"));
}
}
Then use FeatureLoader.load(...) instead of ServiceLoader.load(...) for both engine and command.
| public interface FeatureSpi | ||
| { | ||
| String name(); | ||
| } |
There was a problem hiding this comment.
We probably don't need this class if we use FeatureLoader approach instead.
| * License for the specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package io.aklivity.zilla.runtime.common.annotation; |
There was a problem hiding this comment.
Suggest we move this alongside FeatureLoader in io.aklivity.zilla.runtime.common.feature package.
| exports io.aklivity.zilla.runtime.common; | ||
|
|
||
| uses io.aklivity.zilla.runtime.common.CommonSpi; |
There was a problem hiding this comment.
This will change with the repackaging.
|
|
||
| assertEquals(1, status); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we need ZillaMain tests in common? Seems like the dependency is inverted.
| if (!service.getClass().getPackageName().contains("incubator") || | ||
| incubatorEnabled) | ||
| { | ||
| service.mixin(builder); | ||
| } |
There was a problem hiding this comment.
Use FeatureLoader.load(...) instead, so no need for if condition around service.mixin(...) call.
| import java.util.function.Predicate; | ||
| import java.util.stream.StreamSupport; | ||
|
|
||
| public final class FeatureLoader |
| public static final boolean INCUBATOR_ENABLED = incubatorEnabled(); | ||
| public static final Predicate<Object> FEATURE_ENABLED = FeatureLoader::featureEnabled; |
There was a problem hiding this comment.
| public static final boolean INCUBATOR_ENABLED = incubatorEnabled(); | |
| public static final Predicate<Object> FEATURE_ENABLED = FeatureLoader::featureEnabled; | |
| private static final boolean INCUBATOR_ENABLED = incubatorEnabled(); | |
| private static final Predicate<Object> FEATURE_ENABLED = FeatureLoader::featureEnabled; |
| Iterable<S> factories, | ||
| Function<Map<String, S>, F> construct) | ||
| { | ||
| Map<String, S> factoriesByName = new TreeMap<>(); |
| Function<Map<String, S>, F> construct) | ||
| { | ||
| Map<String, S> factoriesByName = new TreeMap<>(); | ||
| for (S factory : factories) |
There was a problem hiding this comment.
Recommend we apply the filter(...) call here around factories instead of in each subclass.
Description
Support incubator features preview in zilla release docker image.
Fixes #670