Ryan0751 commented on code in PR #396:
URL: https://github.com/apache/curator/pull/396#discussion_r1161287446
##########
curator-x-async/src/main/java/org/apache/curator/x/async/modeled/details/ModeledCacheImpl.java:
##########
@@ -188,9 +188,18 @@ private void internalChildEvent(TreeCacheEvent event)
{
ZPath path = ZPath.parse(event.getData().getPath());
Entry<T> entry = entries.remove(path);
- T model = (entry != null) ? entry.model :
serializer.deserialize(event.getData().getData());
- Stat stat = (entry != null) ? entry.stat :
event.getData().getStat();
- accept(ModeledCacheListener.Type.NODE_REMOVED, path, stat, model);
+ T model = null;
+ if (entry != null) {
+ model = entry.model;
+ }
+ else if (event.getData().getData() != null) {
+ model = serializer.deserialize(event.getData().getData());
+ }
+ if (model != null) {
+ Stat stat = (entry != null) ? entry.stat :
event.getData().getStat();
+ accept(ModeledCacheListener.Type.NODE_REMOVED, path, stat,
model);
Review Comment:
Thank you for the explanation of ModelSerializer.raw (I totally missed that
it was defined in an interface).
As for your second suggestion (not calling deserialized and
accept/handleException for intermediate nodes in the case of NODE_REMOVED),
does that present a bigger risk of breaking existing users if they expect those
nodes to contain valid models?
##########
curator-x-async/src/test/java/org/apache/curator/x/async/modeled/TestCachedModeledFramework.java:
##########
@@ -181,6 +184,72 @@ public void testAccessCacheDirectly()
}
}
+ // Verify the CachedModeledFramework does not attempt to deserialize empty
ZNodes on deletion.
+ // See: CURATOR-609
+ @Test
+ public void testEmptyNodeDeserialization()
+ {
+ // The sub-path is the ZNode that will be removed that does not
contain a model. This model with no data
+ // should not result in attempt to deserialize.
+ final String subPath = modelSpec.path().toString() + "/sub";
+
+ final String firstModelPath = subPath + "/first";
+ final String secondModelPath = subPath + "/second";
+
+ CountDownLatch latch = new CountDownLatch(1);
+
+ final AtomicBoolean caughtException = new AtomicBoolean(false);
+
+ // Create a custom listener to signal the end of the test and ensure
no exceptions are thrown.
+ final ModeledCacheListener<TestModel> listener = new
ModeledCacheListener<TestModel>() {
+ @Override
+ public void accept(Type t, ZPath p, Stat s, TestModel m)
+ {
+ if (t == ModeledCacheListener.Type.NODE_ADDED &&
p.toString().equals(secondModelPath)) {
+ latch.countDown();
+ }
+ }
+
+ public void handleException(Exception e) {
+ System.err.printf("Caught unexpected exception %s%n", e);
Review Comment:
I like storing the Exception. I tried to assertFail right there, but the
exception is swallowed somewhere in the implementation and didn't cause the
test to fail.
--
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: commits-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
