kkonstantine commented on a change in pull request #8978:
URL: https://github.com/apache/kafka/pull/8978#discussion_r449657463
##########
File path: clients/src/test/java/org/apache/kafka/test/MockDeserializer.java
##########
@@ -26,17 +26,16 @@
public class MockDeserializer implements ClusterResourceListener,
Deserializer<byte[]> {
public static AtomicInteger initCount = new AtomicInteger(0);
- public static AtomicInteger closeCount = new AtomicInteger(0);
public static AtomicReference<ClusterResource> clusterMeta = new
AtomicReference<>();
public static ClusterResource noClusterId = new
ClusterResource("no_cluster_id");
public static AtomicReference<ClusterResource> clusterIdBeforeDeserialize
= new AtomicReference<>(noClusterId);
public boolean isKey;
public Map<String, ?> configs;
+ public boolean isClosed = false;
Review comment:
This needs to be `volatile` or otherwise accessed with synchronization.
In multicores (and multiprocessors in general) changing the value of a
variable like this is, is not guaranteed to be visible to other threads unless
there's a memory barrier associated with this change, that will enforce a
happens-before behavior for the changes on this variable.
I'm saying the above, without really checking whether this code is accessed
by multiple threads. Yet the presence of several atomic variables tells me that
it is.
##########
File path: clients/src/test/java/org/apache/kafka/test/MockDeserializer.java
##########
@@ -26,17 +26,16 @@
public class MockDeserializer implements ClusterResourceListener,
Deserializer<byte[]> {
public static AtomicInteger initCount = new AtomicInteger(0);
- public static AtomicInteger closeCount = new AtomicInteger(0);
public static AtomicReference<ClusterResource> clusterMeta = new
AtomicReference<>();
public static ClusterResource noClusterId = new
ClusterResource("no_cluster_id");
public static AtomicReference<ClusterResource> clusterIdBeforeDeserialize
= new AtomicReference<>(noClusterId);
public boolean isKey;
public Map<String, ?> configs;
+ public boolean isClosed = false;
Review comment:
On another note, why are we switching to `boolean` from `integer`? I
agree that there's no use of `closeCount` in the code base and that this is not
public interface. But this changes the symmetry with `initCount` (which remains
an integer) and it could still be used by a third-party package that uses these
tests (by incorrectly depending on non-public interface). My point is, if we
just do this for an assertion, we could use
`closeCount.get() > 0` or `close.get() == 0` for the equivalent of `true` or
`false`
and be on the safe side (preserving symmetry too).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
