sergehuber commented on code in PR #776:
URL: https://github.com/apache/unomi/pull/776#discussion_r3433748241
##########
services/src/test/java/org/apache/unomi/services/impl/TestEventAdmin.java:
##########
@@ -125,9 +117,13 @@ public class TestEventAdmin implements EventAdmin {
*/
private final List<Event> sentEvents = new CopyOnWriteArrayList<>();
+ /**
+ * Creates a TestEventAdmin with a single-threaded executor for ordered
event delivery.
+ */
public TestEventAdmin() {
- this.asyncExecutor = Executors.newCachedThreadPool(r -> {
- Thread t = new Thread(r, "TestEventAdmin-Handler-" +
System.identityHashCode(this));
+ // Use single-threaded executor to guarantee ordered delivery
+ this.asyncExecutor = Executors.newSingleThreadExecutor(r -> {
+ Thread t = new Thread(r, "TestEventAdmin-Async-" +
System.identityHashCode(this));
t.setDaemon(true);
return t;
});
Review Comment:
Valid. Each handler worker blocks indefinitely on `queue.take()`, so a
single-threaded executor starves all handlers registered after the first.
Reverted to `newCachedThreadPool` — per-handler ordering is already guaranteed
by the per-handler `BlockingQueue`, so the cached pool is correct here.
##########
services/src/test/java/org/apache/unomi/services/impl/TestEventAdmin.java:
##########
@@ -426,24 +418,23 @@ public int getHandlerCount() {
*/
public boolean waitForEventProcessing(long timeoutMs) {
long deadline = System.currentTimeMillis() + timeoutMs;
-
- // Wait until all queues are drained AND all in-flight handleEvent
calls have returned.
- // Checking only queue.isEmpty() is insufficient: workers remove
events from the queue
- // before calling handleEvent, so the queue can appear empty while
processing is ongoing.
- while (System.currentTimeMillis() < deadline) {
- boolean allQueuesEmpty =
handlerQueues.values().stream().allMatch(BlockingQueue::isEmpty);
- if (allQueuesEmpty && inFlightCount.get() == 0) {
- return true;
+
+ // Wait for all handler queues to be empty
+ for (BlockingQueue<Event> queue : handlerQueues.values()) {
+ while (!queue.isEmpty() && System.currentTimeMillis() < deadline) {
+ try {
+ Thread.sleep(10);
+ } catch (InterruptedException e) {
Review Comment:
Valid. Workers dequeue an event before calling `handleEvent`, so an empty
queue doesn't mean processing is complete. Restored the original approach:
added an `inFlightCount` `AtomicInteger` that is incremented before
`handleEvent` and decremented in a `finally` block, and
`waitForEventProcessing` now checks both `queue.isEmpty()` and
`inFlightCount.get() == 0` before returning `true`.
##########
services/src/test/java/org/apache/unomi/services/impl/TestConditionEvaluators.java:
##########
@@ -83,15 +86,9 @@ private static ConditionEvaluator
createBooleanConditionEvaluator() {
return (condition, item, context, dispatcher) -> {
tracer.startOperation("boolean", "Evaluating boolean condition
with operator: " + condition.getParameter("operator"), condition);
String operator = (String) condition.getParameter("operator");
- Object subConditionsParam =
condition.getParameter("subConditions");
- if (!(subConditionsParam instanceof List)) {
- tracer.endOperation(true, "No subconditions found, returning
true");
- return true;
- }
- @SuppressWarnings("unchecked")
- List<Condition> subConditions = (List<Condition>)
subConditionsParam;
+ List<Condition> subConditions = (List<Condition>)
condition.getParameter("subConditions");
- if (subConditions.isEmpty()) {
+ if (subConditions == null || subConditions.isEmpty()) {
tracer.endOperation(true, "No subconditions found, returning
true");
Review Comment:
Valid. Removing the instanceof check was a mistake — if the parameter is
absent or has an unexpected type, the direct cast throws `ClassCastException`
instead of treating it as "no subconditions". Restored the original `instanceof
List` guard with the `@SuppressWarnings("unchecked")` cast.
##########
services/src/test/java/org/apache/unomi/services/impl/TestConditionEvaluators.java:
##########
@@ -373,13 +370,8 @@ private static boolean evaluateBetweenCondition(Object
actualValue, Condition co
private static boolean evaluateDateCondition(Object actualValue, Object
expectedValueDate,
Object expectedValueDateExpr,
String operator) {
Object expectedDate = expectedValueDate == null ?
expectedValueDateExpr : expectedValueDate;
- Date actualDateVal = getDate(actualValue);
- Date expectedDateVal = getDate(expectedDate);
- if (actualDateVal == null || expectedDateVal == null) {
- return false;
- }
- boolean isSameDay =
YEAR_MONTH_DAY_FORMAT.format(actualDateVal.toInstant())
-
.equals(YEAR_MONTH_DAY_FORMAT.format(expectedDateVal.toInstant()));
+ boolean isSameDay = yearMonthDayDateFormat.format(getDate(actualValue))
+ .equals(yearMonthDayDateFormat.format(getDate(expectedDate)));
return operator.equals("isDay") ? isSameDay : !isSameDay;
Review Comment:
Valid. `yearMonthDayDateFormat.format(getDate(...))` throws
`NullPointerException` when `DateUtils.getDate` returns null for an unparseable
input. Restored the original null checks for both `actualDateVal` and
`expectedDateVal`, returning `false` when either is null.
--
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]
