Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/main/java/com/castletrade/oms/core/domain/model/Order.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
*/
@Data
@Builder
@NoArgsConstructor
@AllArgsConstructor
public class Order {
private String id;
private String symbol;
Expand All @@ -21,6 +23,21 @@ public class Order {
private OrderStatus status;
private LocalDateTime timestamp;

/**
* Resets the order state for reuse in an object pool.
*/
public void clear() {
this.id = null;
this.symbol = null;
this.assetClass = null;
this.side = null;
this.quantity = null;
this.price = null;
this.executionType = null;
this.status = null;
this.timestamp = null;
Comment on lines +31 to +40
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Considera restringir la visibilidad de clear() para limitar su mal uso fuera de los contextos de pool.

Como método público, clear() permite que cualquier consumidor reinicie un Order que puede seguir en uso, lo que puede causar errores sutiles (por ejemplo, entradas en colecciones que de repente pierden sus campos de identidad). Dado que existe para el uso con el pool, considera hacerlo con visibilidad de paquete (package‑private) o limitar su visibilidad de otra forma de modo que solo OrderPool pueda llamarlo.

Implementación sugerida:

    /**
     * Resets the order state for reuse in an object pool.
     * Package-private to prevent misuse outside pooling contexts.
     */
    void clear() {
        this.id = null;
        this.symbol = null;
        this.assetClass = null;
        this.side = null;
        this.quantity = null;
        this.price = null;
  1. Asegúrate de que cualquier código (por ejemplo, OrderPool) que invoque clear() resida en el mismo paquete com.castletrade.oms.core.domain.model o ajusta su paquete en consecuencia.
  2. Si hay tests que llaman a order.clear() desde un paquete diferente, mueve esos tests al mismo paquete o accede a través de la abstracción de pooling en lugar de llamar a clear() directamente.
Original comment in English

suggestion (bug_risk): Consider restricting clear() visibility to limit misuse outside pooling contexts.

As a public method, clear() lets any caller reset an Order that may still be in use, which can cause subtle bugs (e.g., entries in collections suddenly losing their identity fields). Since it exists for pooling, consider making it package‑private or otherwise limiting visibility so only OrderPool can call it.

Suggested implementation:

    /**
     * Resets the order state for reuse in an object pool.
     * Package-private to prevent misuse outside pooling contexts.
     */
    void clear() {
        this.id = null;
        this.symbol = null;
        this.assetClass = null;
        this.side = null;
        this.quantity = null;
        this.price = null;
  1. Ensure any code (e.g., OrderPool) that invokes clear() resides in the same package com.castletrade.oms.core.domain.model or adjust its package accordingly.
  2. If there are tests calling order.clear() from a different package, either move those tests into the same package or access it via the pooling abstraction instead of directly calling clear().

}

public enum AssetClass {
EQUITY, FX, CRYPTO, FIXED_INCOME
}
Expand Down
61 changes: 61 additions & 0 deletions src/main/java/com/castletrade/oms/core/domain/model/OrderPool.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package com.castletrade.oms.core.domain.model;

import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Component;

import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;

/**
* High-performance object pool for Order objects.
* Designed to minimize GC pressure in high-frequency trading scenarios.
*/
@Slf4j
@Component
public class OrderPool {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Considera introducir una interfaz OrderProvider y una implementación alternativa sencilla para que el pooling quede oculto tras una abstracción y pueda sustituirse sin tocar el código consumidor.

Puedes mantener la optimización de pooling pero desacoplarla del dominio y facilitar su sustitución por una implementación más simple.

1. Introduce una interfaz OrderProvider

Define una abstracción mínima de la que dependan los consumidores en lugar de depender directamente de OrderPool:

package com.castletrade.oms.core.domain.model;

public interface OrderProvider {
    Order borrowOrder();
    void returnOrder(Order order);
}

Después, adapta OrderPool para que la implemente:

@Slf4j
@Component
public class OrderPool implements OrderProvider {
    // existing implementation unchanged
}

Todos los consumidores actuales deberían actualizarse para depender de OrderProvider en lugar de OrderPool:

// before
private final OrderPool orderPool;

// after
private final OrderProvider orderProvider;
// before
Order order = orderPool.borrowOrder();
orderPool.returnOrder(order);

// after
Order order = orderProvider.borrowOrder();
orderProvider.returnOrder(order);

2. Añade una implementación trivial sin pool

Puedes proporcionar una implementación sencilla, basada solo en asignaciones, para casos en los que el pooling no sea necesario (o para tests), sin tocar la lógica existente:

@Component
@Primary // if you want this as the default
public class SimpleOrderProvider implements OrderProvider {

    @Override
    public Order borrowOrder() {
        return new Order();
    }

    @Override
    public void returnOrder(Order order) {
        // nothing to do
    }
}

Después, OrderPool puede usarse solo donde realmente se necesite, o habilitarse mediante configuración/perfil:

@Component
@Profile("pooled-orders")
public class OrderPool implements OrderProvider {
    // current implementation
}

Esto mantiene intacto todo el comportamiento actual de OrderPool, pero:

  • Mueve la complejidad detrás de una interfaz estrecha.
  • Te permite mantener el modelo de dominio centrado en Order en lugar de en detalles de pooling.
  • Permite simplificar o incluso eliminar el pooling más adelante sin tocar los puntos de uso.
Original comment in English

issue (complexity): Consider introducing an OrderProvider interface and simple alternative implementation so that pooling is hidden behind an abstraction and can be swapped out without touching calling code.

You can keep the pooling optimization but decouple it from the domain and make it easier to swap out with a simpler implementation.

1. Introduce an OrderProvider interface

Define a minimal abstraction that callers depend on instead of OrderPool directly:

package com.castletrade.oms.core.domain.model;

public interface OrderProvider {
    Order borrowOrder();
    void returnOrder(Order order);
}

Then adapt OrderPool to implement this:

@Slf4j
@Component
public class OrderPool implements OrderProvider {
    // existing implementation unchanged
}

All current consumers should be updated to depend on OrderProvider instead of OrderPool:

// before
private final OrderPool orderPool;

// after
private final OrderProvider orderProvider;
// before
Order order = orderPool.borrowOrder();
orderPool.returnOrder(order);

// after
Order order = orderProvider.borrowOrder();
orderProvider.returnOrder(order);

2. Add a trivial, non-pooled implementation

You can provide a simple, allocation-only implementation for cases where pooling isn’t needed (or for tests), without touching existing logic:

@Component
@Primary // if you want this as the default
public class SimpleOrderProvider implements OrderProvider {

    @Override
    public Order borrowOrder() {
        return new Order();
    }

    @Override
    public void returnOrder(Order order) {
        // nothing to do
    }
}

Then OrderPool can be used only where really needed, or enabled via configuration/profile:

@Component
@Profile("pooled-orders")
public class OrderPool implements OrderProvider {
    // current implementation
}

This keeps all current OrderPool behavior intact but:

  • Moves complexity behind a narrow interface.
  • Lets you keep the domain model focused on Order rather than pooling details.
  • Allows you to simplify or even remove pooling later without touching call sites.

private final BlockingQueue<Order> pool;
private static final int DEFAULT_POOL_SIZE = 1000;

public OrderPool() {
this(DEFAULT_POOL_SIZE);
}

public OrderPool(int size) {
this.pool = new ArrayBlockingQueue<>(size);
for (int i = 0; i < size; i++) {
pool.add(new Order());
}
log.info("Initialized OrderPool with {} pre-allocated objects", size);
}

/**
* Borrows an order from the pool.
*/
public Order borrowOrder() {
Order order = pool.poll();
if (order == null) {
log.warn("OrderPool exhausted, creating new transient object. Consider increasing pool size.");
return new Order();
}
Comment on lines +31 to +39
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (performance): Usar WARN para el agotamiento del pool en una ruta caliente puede generar un exceso de ruido en los logs bajo carga.

En escenarios de alto rendimiento, esta condición puede producirse con frecuencia, así que WARN aquí puede tanto ralentizar la ruta caliente como inundar los logs. Considera bajar el nivel a INFO/DEBUG o añadir limitación de frecuencia (por ejemplo, registrar cada N ocurrencias o en un intervalo de tiempo) para mantener la observabilidad sin un E/S de logs excesivo.

Suggested change
/**
* Borrows an order from the pool.
*/
public Order borrowOrder() {
Order order = pool.poll();
if (order == null) {
log.warn("OrderPool exhausted, creating new transient object. Consider increasing pool size.");
return new Order();
}
/**
* Borrows an order from the pool.
*/
public Order borrowOrder() {
Order order = pool.poll();
if (order == null) {
log.debug("OrderPool exhausted, creating new transient object. Consider increasing pool size.");
return new Order();
}
Original comment in English

suggestion (performance): Using WARN for pool exhaustion in a hot path may generate excessive log noise under load.

In high-throughput scenarios, this condition may be hit frequently, so WARN here can both slow the hot path and flood logs. Consider lowering the level to INFO/DEBUG or adding throttling (e.g., log every Nth occurrence or on a time interval) to preserve observability without excessive log I/O.

Suggested change
/**
* Borrows an order from the pool.
*/
public Order borrowOrder() {
Order order = pool.poll();
if (order == null) {
log.warn("OrderPool exhausted, creating new transient object. Consider increasing pool size.");
return new Order();
}
/**
* Borrows an order from the pool.
*/
public Order borrowOrder() {
Order order = pool.poll();
if (order == null) {
log.debug("OrderPool exhausted, creating new transient object. Consider increasing pool size.");
return new Order();
}

return order;
}

/**
* Returns an order to the pool after resetting its state.
*/
public void returnOrder(Order order) {
if (order == null) return;
order.clear();
boolean accepted = pool.offer(order);
if (!accepted) {
log.debug("OrderPool full, discarding order object.");
Comment on lines +46 to +51
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Devolver la misma instancia de Order varias veces puede llevar a múltiples referencias al mismo objeto en el pool.

Dado que ArrayBlockingQueue permite duplicados, el mismo Order puede añadirse al pool varias veces si se llama a returnOrder dos veces sobre él (o sobre un objeto que nunca se pidió prestado). Eso puede llevar a que la misma instancia mutable se entregue a múltiples consumidores simultáneamente. Si esto es posible en tu caso de uso, añade una protección (por ejemplo, un flag inPool en Order o una pequeña estructura de seguimiento) para evitar devoluciones dobles u objetos externos que se devuelvan.

Original comment in English

issue (bug_risk): Returning the same Order instance multiple times can lead to multiple references to the same object in the pool.

Since ArrayBlockingQueue allows duplicates, the same Order can be added to the pool multiple times if returnOrder is called twice on it (or on an object never borrowed). That can lead to the same mutable instance being handed to multiple borrowers concurrently. If this is possible in your usage, add a guard (e.g., an inPool flag on Order or a small tracking structure) to prevent double returns or foreign objects being returned.

}
}

/**
* Returns the current number of available objects in the pool.
*/
public int getAvailableCount() {
return pool.size();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package com.castletrade.oms.core.domain.model;

import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.*;

class OrderPoolTest {
Comment thread
sourcery-ai[bot] marked this conversation as resolved.

@Test
void testBorrowAndReturn() {
OrderPool pool = new OrderPool(10);
assertEquals(10, pool.getAvailableCount());

Order order = pool.borrowOrder();
assertNotNull(order);
assertEquals(9, pool.getAvailableCount());

order.setSymbol("BTC/USD");
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
pool.returnOrder(order);
Comment thread
sourcery-ai[bot] marked this conversation as resolved.

assertEquals(10, pool.getAvailableCount());

Order reborrowed = pool.borrowOrder();
assertNull(reborrowed.getSymbol(), "Order should be cleared when returned to pool");
}

@Test
void testPoolExhaustion() {
OrderPool pool = new OrderPool(1);
pool.borrowOrder();

Order exhausted = pool.borrowOrder();
assertNotNull(exhausted, "Should create fresh object when pool is empty");
assertEquals(0, pool.getAvailableCount());
}
}
Loading