oscerd commented on code in PR #27440: URL: https://github.com/apache/camel/pull/27440#discussion_r4204221576
########## components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureEndpoint.java: ########## @@ -0,0 +1,420 @@ +/* + * 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.camel.component.openfeature; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.time.Instant; +import java.util.HashMap; +import java.util.Map; + +import dev.openfeature.contrib.providers.flagd.Config; +import dev.openfeature.contrib.providers.flagd.FlagdOptions; +import dev.openfeature.contrib.providers.flagd.FlagdProvider; +import dev.openfeature.sdk.Client; +import dev.openfeature.sdk.FeatureProvider; +import dev.openfeature.sdk.FlagEvaluationDetails; +import dev.openfeature.sdk.MutableContext; +import dev.openfeature.sdk.Value; +import org.apache.camel.Category; +import org.apache.camel.Consumer; +import org.apache.camel.Exchange; +import org.apache.camel.Processor; +import org.apache.camel.Producer; +import org.apache.camel.spi.Metadata; +import org.apache.camel.spi.UriEndpoint; +import org.apache.camel.spi.UriParam; +import org.apache.camel.spi.UriPath; +import org.apache.camel.support.DefaultEndpoint; +import org.apache.camel.support.ResourceHelper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Evaluate feature flags using the OpenFeature specification with flagd. */ +@UriEndpoint(firstVersion = "4.23.0", scheme = "openfeature", title = "OpenFeature", syntax = "openfeature:domain", + producerOnly = true, category = { Category.CLOUD }, headersClass = OpenFeatureConstants.class) +public class OpenFeatureEndpoint extends DefaultEndpoint { + + private static final Logger LOG = LoggerFactory.getLogger(OpenFeatureEndpoint.class); + private static final String DEFAULT_PROVIDER_BEAN = "flags"; + + @UriPath + @Metadata(required = true) + private String domain; + + @UriParam + private OpenFeatureConfiguration configuration; + + private volatile Client client; + private volatile FeatureProvider resolvedProvider; + private volatile File tempFlagFile; + private volatile boolean ownedProvider; + + public OpenFeatureEndpoint(String uri, OpenFeatureComponent component, String domain, + OpenFeatureConfiguration configuration) { + super(uri, component); + this.domain = domain; + this.configuration = configuration; + } + + @Override + public OpenFeatureComponent getComponent() { + return (OpenFeatureComponent) super.getComponent(); + } + + @Override + public Producer createProducer() { + return new OpenFeatureProducer(this); + } + + @Override + public Consumer createConsumer(Processor processor) { + throw new UnsupportedOperationException("OpenFeature is producer only"); + } + + @Override + protected void doStart() throws Exception { + super.doStart(); + configuration.validate(); + + if (domain == null || domain.isBlank()) { + throw new IllegalArgumentException("domain must not be blank"); + } + + FeatureProvider provider = resolveProvider(); + resolvedProvider = provider; + client = getComponent().registerEndpoint(domain, provider, ownedProvider); + } + + @Override + protected void doStop() throws Exception { + client = null; + resolvedProvider = null; + + getComponent().unregisterEndpoint(domain); Review Comment: **A failed endpoint start still decrements the shared domain refCount** `doStop()` always calls `getComponent().unregisterEndpoint(domain)`, but Camel also runs `doStop()` when `doStart()` throws: `BaseService.start()` catches the exception and calls `stop()`. An endpoint that fails in `configuration.validate()` or `resolveProvider()`, before `registerEndpoint` ran, therefore still decrements the binding the other endpoints hold. For example, with `camel.component.openfeature.flags-resource` set, adding or reloading a route with `to("openfeature:flags?flagKey=x&provider=#typo")` (or with inline `flags=`, which fails validation against the inherited `flagsResource`) drops the `flags` binding to 0 and shuts down the provider still used by every running `openfeature:flags` endpoint and every `language("openfeature", ...)`. With `host` that closes their gRPC channel, so they fall back to `defaultValue`. The count also stays wrong afterwards: the next endpoint registers a fresh provider with `refCount = 1` although the earlier endpoints use it too. Unregis tering only when this endpoint actually registered (e.g. a `registered` flag set after `registerEndpoint` returns, or checking `client != null`) would fix it; a test that starts a second endpoint with `provider=#missing` on a shared domain and then evaluates through the first would pin it. ########## components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureComponent.java: ########## @@ -0,0 +1,127 @@ +/* + * 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.camel.component.openfeature; + +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +import dev.openfeature.sdk.Client; +import dev.openfeature.sdk.FeatureProvider; +import dev.openfeature.sdk.OpenFeatureAPI; +import org.apache.camel.Endpoint; +import org.apache.camel.spi.Metadata; +import org.apache.camel.spi.annotations.Component; +import org.apache.camel.support.DefaultComponent; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Component("openfeature") +public class OpenFeatureComponent extends DefaultComponent { + + private static final Logger LOG = LoggerFactory.getLogger(OpenFeatureComponent.class); + + @Metadata + private OpenFeatureConfiguration configuration = new OpenFeatureConfiguration(); + + private volatile OpenFeatureAPI api; + private final Map<String, DomainBinding> domainBindings = new ConcurrentHashMap<>(); + + private static class DomainBinding { + final FeatureProvider provider; + final boolean owned; + int refCount; + + DomainBinding(FeatureProvider provider, boolean owned) { + this.provider = provider; + this.owned = owned; + this.refCount = 1; + } + } + + @Override + protected Endpoint createEndpoint(String uri, String remaining, Map<String, Object> parameters) throws Exception { + OpenFeatureEndpoint endpoint = new OpenFeatureEndpoint(uri, this, remaining, configuration.copy()); + setProperties(endpoint, parameters); + return endpoint; + } + + @Override + protected void doStart() throws Exception { + super.doStart(); + api = OpenFeatureAPI.createIsolated(); + } + + @Override + protected void doStop() throws Exception { + api = null; Review Comment: **Bean providers stay registered to the discarded API, so re-registering them fails** Not shutting down `provider=#bean` / `flags` beans is right, but SDK 1.22.1 records every registered provider in a static registry that is only released when the provider is replaced or when `api.shutdown()` runs. Now that `doStop()` just drops the isolated API, a bean provider stays registered to the old instance, and the next isolated API that sees the same bean throws `IllegalStateException: Provider ... is already registered with another API instance` (spec 1.8.4), e.g. a static provider fixture shared by several `CamelTestSupport` test methods, or a CamelContext stop/start such as the JMX `restart`. Letting the application pass its own `OpenFeatureAPI` (never shut down by Camel), and calling `api.shutdown()` only on the API the component created, would cover both this and the remaining half of the earlier point. ########## components/camel-openfeature/src/main/java/org/apache/camel/component/openfeature/OpenFeatureComponent.java: ########## @@ -0,0 +1,127 @@ +/* + * 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.camel.component.openfeature; + +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +import dev.openfeature.sdk.Client; +import dev.openfeature.sdk.FeatureProvider; +import dev.openfeature.sdk.OpenFeatureAPI; +import org.apache.camel.Endpoint; +import org.apache.camel.spi.Metadata; +import org.apache.camel.spi.annotations.Component; +import org.apache.camel.support.DefaultComponent; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Component("openfeature") +public class OpenFeatureComponent extends DefaultComponent { + + private static final Logger LOG = LoggerFactory.getLogger(OpenFeatureComponent.class); + + @Metadata + private OpenFeatureConfiguration configuration = new OpenFeatureConfiguration(); + + private volatile OpenFeatureAPI api; + private final Map<String, DomainBinding> domainBindings = new ConcurrentHashMap<>(); + + private static class DomainBinding { + final FeatureProvider provider; + final boolean owned; + int refCount; + + DomainBinding(FeatureProvider provider, boolean owned) { + this.provider = provider; + this.owned = owned; + this.refCount = 1; + } + } + + @Override + protected Endpoint createEndpoint(String uri, String remaining, Map<String, Object> parameters) throws Exception { + OpenFeatureEndpoint endpoint = new OpenFeatureEndpoint(uri, this, remaining, configuration.copy()); + setProperties(endpoint, parameters); + return endpoint; + } + + @Override + protected void doStart() throws Exception { + super.doStart(); + api = OpenFeatureAPI.createIsolated(); + } + + @Override + protected void doStop() throws Exception { + api = null; + for (DomainBinding binding : domainBindings.values()) { + if (binding.owned) { + try { + binding.provider.shutdown(); + } catch (Exception e) { + LOG.debug("Error shutting down owned provider: {}", e.getMessage(), e); + } + } + } + domainBindings.clear(); + super.doStop(); + } + + synchronized Client registerEndpoint(String domain, FeatureProvider provider, boolean owned) throws Exception { + DomainBinding existing = domainBindings.get(domain); + if (existing != null) { + existing.refCount++; + if (existing.provider != provider) { + LOG.warn("Domain '{}' already has a registered provider; this endpoint's provider settings are ignored.", + domain); + if (owned) { + provider.shutdown(); Review Comment: **Extra endpoints on a domain still build a provider; shutting down an unused FlagdProvider does not close its gRPC channel** `resolveProvider()` still runs before the binding is checked, so every additional endpoint on a domain builds its own provider, and releasing it here does not help in host mode: flagd 0.14.2's `FlagdProvider.shutdown()` returns immediately for a never-initialized provider, while its constructor has already built the gRPC `ManagedChannel`, so the channel leaks. The WARN also compares provider instances rather than settings, so it fires for every such endpoint even when all of them inherit the same component settings (the docs' pattern, and `OpenFeatureLanguageTest` triggers it), which buries a real conflict such as a different `flagsResource` on the same domain. Looking up the binding first and resolving a provider only for the first endpoint of a domain would fix all three (related to davsclaus's thread asking for the same lookup-first change). -- 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]
