kezhuw commented on code in PR #396:
URL: https://github.com/apache/curator/pull/396#discussion_r1159978743
##########
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:
> Previously, `accept(ModeledCacheListener.Type.NODE_REMOVED...` would
always be called.
It is only true for `ModelSerializer.raw` which I think is a unusual usage
of modeled framework. With `JacksonModelSerializer`, the `deserialize` will
fail hence `ModeledCacheListener.accept` was not called.
I think it is a chance to unify the two. I prefer the second option as there
is no sense to accept a `null` model (`raw` or `json`) for intermediate nodes
in all event types. Consistency should play more importance here than
potential breaking of `ModelSerializer.raw`.
@Randgalt What do you think ?
--
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]
