refactor: trim comments

Author: Zaimwa9

src/main/java/com/flagsmith/FlagsmithClient.java

@@ -203,12 +203,7 @@ public List<Segment> getIdentitySegments(String identifier, Map<String, Object>
   }
 
   /**
-   * Resolve a flag for an identity and record one {@code $flag_exposure} event when the identity
-   * is enrolled in a running experiment on that feature. Identity flags are fetched exactly as
-   * {@link #getIdentityFlags(String)} fetches them.
-   *
-   * <p>Experiment metadata is only carried by remote evaluation. With local evaluation or offline
-   * mode the flag is still returned but no exposure is recorded.
+   * As {@link #getExperimentFlag(String, String, Map)}, with no traits.
    *
    * @param featureName feature name
    * @param identifier  identifier string
@@ -223,12 +218,9 @@ public BaseFlag getExperimentFlag(String featureName, String identifier)
   }
 
   /**
-   * Resolve a flag for an identity and record one {@code $flag_exposure} event when the identity
-   * is enrolled in a running experiment on that feature. Identity flags are fetched exactly as
-   * {@link #getIdentityFlags(String, Map)} fetches them.
-   *
-   * <p>Experiment metadata is only carried by remote evaluation. With local evaluation or offline
-   * mode the flag is still returned but no exposure is recorded.
+   * Get an identity's flag, recording one {@code $flag_exposure} event if the identity is enrolled
+   * in a running experiment on it. Only remote evaluation carries experiment metadata, so local
+   * evaluation and offline mode record no exposure.
    *
    * @param featureName feature name
    * @param identifier  identifier string
@@ -246,8 +238,7 @@ public BaseFlag getExperimentFlag(
     Flags flags = getIdentityFlags(identifier, traits);
 
     if (flags == null) {
-      // The API wrapper returns null rather than throwing when the identities request times out
-      // or is interrupted. Serve the default flag, as getFlag would with the API unavailable.
+      // The API wrapper returns null, not throws, on a timed-out or interrupted request.
       FlagsmithFlagDefaults defaults = getConfig().getFlagsmithFlagDefaults();
       if (defaults == null) {
         throw new FlagsmithApiError("Failed to get feature flags.");

src/main/java/com/flagsmith/config/FlagsmithConfig.java

@@ -299,9 +299,8 @@ public Builder withEnableAnalytics(Boolean enable) {
     }
 
     /**
-     * Set the base URL of the Flagsmith events API, overriding the default one. Harmless when
-     * events are not enabled, so that a shared configuration can carry the URL for the services
-     * that do enable them.
+     * Override the events API base URL. Allowed with events disabled, so a shared configuration
+     * can carry it.
      *
      * @param eventsUri the new base URI for the events API
      * @return the Builder
@@ -325,9 +324,9 @@ public Builder withEnableEvents(Boolean enable) {
     }
 
     /**
-     * Set the event processor.
+     * Use a custom event processor. Also enables events.
      *
-     * @param processor event processor object
+     * @param processor the processor that buffers and sends events
      * @return the Builder
      */
     public Builder withEventProcessor(EventProcessor processor) {

src/main/java/com/flagsmith/config/Retry.java

@@ -26,11 +26,9 @@ public class Retry {
       add(503);
     }};
   /**
-   * When true, a response is only retried if its status code is in {@link #statusForcelist}, and
-   * never beyond the {@link #total} attempts budget. A connection failure (a null status code) is
-   * still retried while attempts remain. Defaults to false, which keeps the historical behaviour
-   * of retrying any status while attempts remain, and retrying a force-listed status regardless of
-   * the budget.
+   * When true, only force-listed statuses and connection failures (null status) are retried, and
+   * never past {@link #total} attempts. False keeps the historical policy: any status retries
+   * within the budget, and a force-listed one regardless of it.
    */
   private Boolean statusForcelistOnly = Boolean.FALSE;
 
@@ -39,7 +37,7 @@ public Retry(Integer total) {
   }
 
   /**
-   * Instantiate without a status forcelist policy, which keeps the historical behaviour.
+   * Create a policy with {@code statusForcelistOnly} off.
    *
    * @param total           number of attempts before giving up
    * @param attempts        attempts made so far

src/main/java/com/flagsmith/threads/EventProcessor.java

@@ -39,10 +39,9 @@
 /**
  * Buffers experimentation events and ships them to the Flagsmith events API.
  *
- * <p>Events are flushed on a fixed interval, when the buffer fills up, and on {@link #close()}.
- * Exposure events are deduplicated within a flush window. No exception thrown here reaches caller
- * code: every failure is logged instead, including traits or metadata that cannot be serialised,
- * which drop only their own event. {@link Error}s such as {@code OutOfMemoryError} are not caught.
+ * <p>Events are flushed on a timer, when the buffer fills, and on {@link #close()}. Exposures are
+ * deduplicated within a flush window. Exceptions are logged, never thrown to the caller;
+ * {@link Error}s are not caught.
  */
 public class EventProcessor {
 
@@ -58,15 +57,9 @@ public class EventProcessor {
   private static final MediaType JSON_MEDIA_TYPE =
       MediaType.get("application/json; charset=utf-8");
   /**
-   * The most events that may be waiting on the events API at once. Once reached, a flush drops
-   * its batch instead of queueing it: while the API is slow or down, batches are produced faster
-   * than they are retried and given up on, and an unbounded queue of them would grow with the
-   * host application's traffic until it ran out of memory.
-   *
-   * <p>It counts events, not batches, so that it bounds memory without capping throughput on a
-   * healthy API: a batch count would throttle harder the smaller the configured buffer. At the
-   * default buffer size it is ten batches. A batch is admitted while the count is below the
-   * limit, so the bound is this plus one buffer's worth.
+   * Cap on events awaiting the events API. Past it a flush drops its batch rather than queue it,
+   * so an outage cannot grow memory with the host's traffic. It counts events, not batches, so a
+   * small buffer is not throttled on a healthy API. The true bound is this plus one buffer.
    */
   static final int MAX_IN_FLIGHT_EVENTS = 10_000;
   /** The close timeout when the HTTP client's timeouts do not bound a request. */
@@ -83,8 +76,7 @@ public class EventProcessor {
   /** The interval between timed flushes; 0 means there is no timer. */
   @Getter
   private final int flushIntervalMillis;
-  // Everything below is internal state, deliberately not exposed: once published, a getter
-  // would be public API for as long as the SDK is supported.
+  // No public getters below: each one would become supported API.
   private final List<Map<String, Object>> buffer = new ArrayList<>();
   private final Set<String> dedupeKeys = new HashSet<>();
   private final Object lock = new Object();
@@ -110,9 +102,9 @@ public class EventProcessor {
   private ScheduledFuture<?> scheduledFlush;
 
   /**
-   * Instantiate with an HTTP client.
+   * Create a processor that sends batches through {@code client}.
    *
-   * @param client               client instance
+   * @param client               HTTP client; its timeouts also bound {@link #close()}
    * @param eventsUri            base URI of the events API, e.g. https://events.api.flagsmith.com/
    * @param maxBufferItems       number of buffered events that triggers an immediate flush; at
    *                             least 1
@@ -126,14 +118,10 @@ public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems
         new RequestProcessor(client, new FlagsmithLogger(), buildRetry()));
   }
 
-  /**
-   * Instantiate with a request processor. Package-private: it exists for tests, and is not
-   * something callers should come to depend on.
-   */
+  /** For tests: sends through the given request processor. */
   EventProcessor(HttpUrl eventsUri, int maxBufferItems, int flushIntervalMillis,
       RequestProcessor requestProcessor) {
-    // Without a positive buffer limit nothing bounds the buffer between timed flushes, and with
-    // the timer off as well it would grow for as long as the process runs.
+    // The buffer limit is the only bound on the buffer between timed flushes, or with no timer.
     if (maxBufferItems < 1) {
       throw new IllegalArgumentException("maxBufferItems must be at least 1.");
     }
@@ -164,10 +152,9 @@ private static Retry buildRetry() {
   }
 
   /**
-   * The longest one batch can take to be delivered or given up on: every attempt the retry
-   * policy allows, each running to the client's timeouts, plus the backoff between them. An
-   * attempt is bounded by the call timeout when one is set, and otherwise by connect, write and
-   * read in turn. When none of those bounds it, neither is the batch.
+   * The longest one batch can take: every attempt the retry policy allows, each at the client's
+   * timeouts, plus backoff. An attempt is bounded by the call timeout if set, else by connect,
+   * write and read in turn.
    *
    * @return the worst case in milliseconds, or {@link #UNBOUNDED}
    */
@@ -194,9 +181,9 @@ static long worstCaseBatchMillis(OkHttpClient client, Retry retry) {
   }
 
   /**
-   * Set the logger used by the processor and by its request processor.
+   * Set the logger, for this processor and its request processor.
    *
-   * @param logger logger instance
+   * @param logger the client's logger, so event failures appear alongside its other output
    */
   public void setLogger(FlagsmithLogger logger) {
     this.logger = logger;
@@ -282,9 +269,8 @@ public synchronized void start() {
     }
 
     if (closed.get()) {
-      // The scheduler is shut down, and scheduling on it would throw. This happens when a
-      // FlagsmithConfig, which owns the processor, is reused after a client built from it was
-      // closed.
+      // Scheduling on the shut-down scheduler would throw. Reached when a FlagsmithConfig is
+      // reused after a client built from it was closed.
       logger.error("Not starting the event processor: it has been closed.");
       return;
     }
@@ -294,10 +280,8 @@ public synchronized void start() {
   }
 
   /**
-   * Stop the flush timer, ship whatever is left on a best-effort basis and release the HTTP
-   * resources. Blocks until in-flight batches settle, for at most the worst case of one batch
-   * under the HTTP client's timeouts and the retry policy; with a timeout switched off there is
-   * no such bound, and it waits as long as the request does.
+   * Stop the timer, flush what is left and release HTTP resources. Blocks until in-flight
+   * batches settle, for at most the worst case of one batch; unbounded if a client timeout is off.
    */
   public void close() {
     synchronized (this) {
@@ -322,8 +306,7 @@ public void close() {
       logger.error("Failed to flush events on close.", e);
     }
 
-    // shutdown(), never shutdownNow(): interrupting a POST mid-flight loses its batch, where
-    // letting it finish delivers it if the JVM stays up long enough.
+    // A graceful shutdown: interrupting a POST mid-flight would lose its batch.
     requestProcessor.close();
   }
 
@@ -344,17 +327,10 @@ private void bufferEvent(String event, String featureName, String identifier, Ob
       eventMetadata.put(SDK_VERSION_KEY, Versions.getVersion());
       final Object experimentId = eventMetadata.get(EXPERIMENT_ID_KEY);
 
-      // Traits and metadata are caller objects of any shape. Serialising them here, rather than
-      // at flush time, means a value Jackson cannot serialise drops this one event (logged
-      // below) instead of failing the whole batch it would later be sent in. The JSON text is
-      // also a deep copy, so a caller mutating a nested map afterwards cannot change a buffered
-      // event, and is more compact to hold than the objects or a JSON tree.
-      //
-      // It must be writeValueAsString, not valueToTree: on a map or list that contains itself,
-      // valueToTree lets a raw StackOverflowError escape, where writeValueAsString reports it as
-      // a JsonMappingException that the catch below handles.
       Map<String, Object> eventTraits = eventTraits(traits);
 
+      // Serialised now, not at flush: an unserialisable value drops only this event, not its
+      // batch, and the JSON text is a deep copy that later caller mutation cannot reach.
       Map<String, Object> eventPayload = new LinkedHashMap<>();
       eventPayload.put("event", event);
       eventPayload.put("feature_name", featureName);
@@ -367,9 +343,8 @@ private void bufferEvent(String event, String featureName, String identifier, Ob
       boolean isFull;
 
       synchronized (lock) {
-        // Checked again under the lock: close() sets the flag before its final flush takes the
-        // lock, so an event either lands in the buffer ahead of that flush or is refused here.
-        // Checking only above would let an event slip in after the final flush and be lost.
+        // Re-checked under the lock: close() sets the flag before its final flush takes the
+        // lock, so an event either makes that flush or is refused here, never stranded.
         if (closed.get()) {
           logClosed(event);
           return;
@@ -390,7 +365,10 @@ private void bufferEvent(String event, String featureName, String identifier, Ob
     }
   }
 
-  /** JSON text Jackson writes out verbatim when the batch is serialised. */
+  /**
+   * JSON text written verbatim into the batch. Uses writeValueAsString, not valueToTree: on a
+   * self-containing map valueToTree throws a raw StackOverflowError instead of an exception.
+   */
   private static RawValue toJson(Object value) throws JsonProcessingException {
     return new RawValue(MapperFactory.getMapper().writeValueAsString(value));
   }
@@ -400,10 +378,8 @@ private void logClosed(String event) {
   }
 
   /**
-   * Flatten a caller trait map into the flat map of trait values the events API expects. Values
-   * wrapped in a {@link TraitConfig} are unwrapped, and traits the caller marked transient are
-   * dropped: transient means "do not persist this against the identity", and an event store keeps
-   * what it is sent.
+   * Unwrap {@link TraitConfig} values and drop transient traits: transient means "do not
+   * persist", and an event store keeps what it is sent.
    */
   private static Map<String, Object> eventTraits(Map<String, Object> traits) {
     if (traits == null) {
@@ -437,13 +413,11 @@ private static String nullToEmpty(String value) {
   }
 
   /**
-   * Hand a batch to the request processor. {@code tracked} is settled exactly once, on every path
-   * out of here: leaving it pending would wedge every later {@link #flush()}, since those wait on
-   * it.
+   * Hand a batch to the request processor. {@code tracked} is settled on every path out: left
+   * pending, it would wedge every later {@link #flush()}.
    */
   private void send(List<Map<String, Object>> batch, CompletableFuture<Void> tracked) {
-    // The completion callback outlives this call by as long as the POST takes, so it captures
-    // the size rather than the list: the serialised request already holds the batch's content.
+    // The callback lives as long as the POST, so it holds the size and lets the list be freed.
     final int batchSize = batch.size();
     boolean submitted = false;
 
@@ -505,10 +479,8 @@ private void settle(CompletableFuture<Void> tracked, int batchSize) {
   }
 
   /**
-   * Count dropped events, and say whether it is time to report them. The first drop is reported
-   * at once; later ones accumulate and are reported at most once per
-   * {@link #DROP_LOG_INTERVAL_NANOS}, so a caller saturating the processor during an outage
-   * cannot turn every flush into an error line. Called under the lock.
+   * Count dropped events. The first drop is reported at once, later ones at most once per
+   * {@link #DROP_LOG_INTERVAL_NANOS}, so an outage does not log per flush. Called under the lock.
    *
    * @return the number of drops to report now, or 0 to stay quiet
    */
@@ -528,10 +500,7 @@ private CompletableFuture<Void> awaitInFlight() {
     return CompletableFuture.allOf(inFlight.toArray(new CompletableFuture[0]));
   }
 
-  /**
-   * A snapshot of the buffer, for tests. Taken under the lock, so a test never iterates the live
-   * list while another thread is appending to it.
-   */
+  /** A snapshot of the buffer for tests, taken under the lock. */
   List<Map<String, Object>> bufferedEvents() {
     synchronized (lock) {
       return new ArrayList<>(buffer);

src/main/java/com/flagsmith/threads/RequestProcessor.java

@@ -84,14 +84,13 @@ public <T> Future<T> executeAsync(
   }
 
   /**
-   * Execute the request in async mode, returning a CompletableFuture so callers can compose on
-   * its completion.
+   * Execute the request in async mode, returning a future callers can compose on.
    *
-   * @param request Request object
-   * @param clazz class type of response
-   * @param doThrow should throw Exception
-   * @param retries no of retries before failing
-   * @param <T> Type inference for the response
+   * @param request request to send
+   * @param clazz type to unmarshal the response body into
+   * @param doThrow whether a failed call completes the future exceptionally
+   * @param retries retry policy, copied for this call
+   * @param <T> response type
    * @return a future completed with the unmarshalled response, or null when the call failed and
    *     doThrow is false
    */

src/test/java/com/flagsmith/FlagsmithClientTest.java

@@ -1028,7 +1028,6 @@ public void testEventsConfigWithoutEnableEventsThrows() {
 
     @Test
     public void testEventsUriWithoutEnableEventsIsHarmless() {
-        // A shared configuration may carry the events URL for the services that do enable events.
         FlagsmithConfig config = FlagsmithConfig.newBuilder()
                 .eventsUri("http://events-uri")
                 .build();

src/test/java/com/flagsmith/FlagsmithRetryTest.java

@@ -89,8 +89,7 @@ public void FlagsmithRetry_statusForcelistOnly_stopsAtTheAttemptsBudget() {
 
     retry.retryAttempted();
     retry.retryAttempted();
-    // This is the branch the one-retry-then-drop guarantee rests on: without it, a permanently
-    // failing endpoint loops forever.
+    // Without this bound a permanently failing endpoint retries forever.
     assertFalse(retry.isRetry(503), "a force-listed status must not retry past the budget");
     assertFalse(retry.isRetry(null), "a connection failure must not retry past the budget");
   }