nrknithin commented on code in PR #6903:
URL: https://github.com/apache/incubator-kie/pull/6903#discussion_r3894205519
##########
optaplanner-build/optaplanner-build-parent/pom.xml:
##########
Review Comment:
yes — it was only there for those micrometer overrides, so it's unused now.
Removed it.
##########
kogito-quarkus/addons/knative/eventing/integration-tests/src/test/java/org/kie/kogito/addons/quarkus/knative/eventing/KSinkInjectionHealthCheckEnabledIT.java:
##########
@@ -39,7 +37,6 @@ class KSinkInjectionHealthCheckEnabledIT extends
AbstractKSinkInjectionHealthChe
@Test
void kSinkInjectionHealthEnabled() {
- assertThat(config.getOptionalValue(CONFIG_ALIAS,
Boolean.class)).hasValue(true);
Review Comment:
This is a QuarkusIntegrationTest, so the app runs in its own process and
ConfigProvider.getConfig() here reads the test JVM's config, not the app's. Up
to Quarkus 3.27 the test framework copied the profile overrides into system
properties, so the test could still see the value — 3.33 doesn't do that
anymore, so getOptionalValue comes back empty and the assertion fails. It was
really only checking a test-harness side effect. The assertHealthChecks call
below still verifies the actual behaviour (health check present when enabled),
so I dropped the config check.
##########
kogito-quarkus/addons/knative/eventing/integration-tests/src/test/java/org/kie/kogito/addons/quarkus/knative/eventing/KSinkInjectionHealthCheckDisabledIT.java:
##########
@@ -40,7 +38,6 @@ class KSinkInjectionHealthCheckDisabledIT extends
AbstractKSinkInjectionHealthCh
@Test
void kSinkInjectionHealthDisabled() {
- assertThat(config.getOptionalValue(CONFIG_ALIAS,
Boolean.class)).hasValue(false);
Review Comment:
Same reason as in the Enabled test — the test JVM doesn't see the profile
override anymore on 3.33, so the config read comes back empty. The health
endpoint assertion below still verifies the check is actually disabled.
##########
kogito-quarkus/addons/knative/eventing/integration-tests/src/test/java/org/kie/kogito/addons/quarkus/knative/eventing/AbstractKSinkInjectionHealthCheckIT.java:
##########
@@ -18,22 +18,12 @@
*/
package org.kie.kogito.addons.quarkus.knative.eventing;
-import org.eclipse.microprofile.config.Config;
-import org.eclipse.microprofile.config.ConfigProvider;
import org.hamcrest.Matcher;
-import org.junit.jupiter.api.BeforeEach;
import static io.restassured.RestAssured.given;
abstract class AbstractKSinkInjectionHealthCheckIT {
- Config config;
-
- @BeforeEach
- void setUp() {
- config = ConfigProvider.getConfig();
Review Comment:
It was only there to feed the config assertions I removed in the subclasses
— nothing else reads it, so the field and the setUp became dead code.
##########
kogito-apps-quarkus/jobs-service-quarkus/jobs-service-infinispan/pom.xml:
##########
@@ -66,10 +66,6 @@
<artifactId>quarkus-infinispan-client</artifactId>
</dependency>
<!-- Needs to be declared as Jobs service doesn't leverage
persistence-commons-infinispan. -->
- <dependency>
- <groupId>org.infinispan</groupId>
- <artifactId>infinispan-query-dsl</artifactId>
- </dependency>
Review Comment:
infinispan-query-dsl doesn't exist anymore in Infinispan 16 (which Quarkus
3.33 pulls in) — the query dsl classes moved into infinispan-commons, which we
already get transitively from quarkus-infinispan-client. The quarkus bom also
stopped managing it, so the build couldn't even resolve it. And you're right
about the comment — it belonged to this dependency, removed it.
##########
kogito-apps-quarkus/jobs-service-quarkus/jobs-service-common/src/main/java/org/kie/kogito/jobs/service/repository/impl/BaseReactiveJobRepository.java:
##########
@@ -43,7 +43,14 @@ protected BaseReactiveJobRepository(Vertx vertx,
JobEventPublisher jobEventPubli
public <T> CompletionStage<T> runAsync(Supplier<T> function) {
final CompletableFuture<T> future = new CompletableFuture<>();
- vertx.executeBlocking(v -> future.complete(function.get()), r -> {
+ vertx.executeBlocking(v -> {
+ try {
+ future.complete(function.get());
+ } catch (Throwable t) {
+ // without this, any failure would leave the future incomplete
and callers waiting forever
+ future.completeExceptionally(t);
+ }
+ }, r -> {
});
Review Comment:
That pattern was already there before this PR — I only added the try/catch
so a failing supplier doesn't leave callers waiting forever. But fair point,
I've switched it to vertx.executeBlocking(function::get).toCompletionStage(),
which is simpler and completes the vertx future properly.
##########
kogito-apps-quarkus/jobs-service-quarkus/jobs-service-infinispan/src/main/java/org/kie/kogito/jobs/service/repository/infinispan/InfinispanConfiguration.java:
##########
@@ -64,6 +70,14 @@ public HealthCheck
infinispanHealthCheck(Instance<RemoteCacheManager> cacheManag
void initializeCaches(@Observes
@Priority(Interceptor.Priority.PLATFORM_BEFORE) StartupEvent startupEvent,
Instance<RemoteCacheManager> remoteCacheManager,
Event<InfinispanInitialized> initializedEvent) {
+ // ProtoStream 6 no longer indexes interface-typed marshallers by Java
class; this provider
+ // keeps Trigger-typed values resolvable (see
TriggerMarshallerProvider).
+ try {
+ MarshallerUtil.getSerializationContext(remoteCacheManager.get())
+ .registerMarshallerProvider(new
TriggerMarshallerProvider());
+ } catch (RuntimeException e) {
+ LOGGER.warn("ProtoStream SerializationContext is not available,
Trigger marshalling may not work: {}", e.getMessage());
Review Comment:
I went with the fail-fast option instead of the logging fix — if the
SerializationContext isn't available, the Infinispan repository can't work at
all (JobDetailsMarshaller is itself ProtoStream-based), so logging a warning
would just delay the failure to the first job write with a much less obvious
error. Removed the try/catch so startup fails immediately with the full stack
trace, which also covers the diagnosability concern. Only extra change was
stubbing getMarshaller() in InfinispanConfigurationTest since its mock used to
go through the catch branch.
--
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]
