rkhachatryan commented on code in PR #23147:
URL: https://github.com/apache/flink/pull/23147#discussion_r1285038776
##########
flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java:
##########
@@ -54,19 +54,21 @@ default StreamStateHandle registerReference(
}
/**
- * Register a reference to the given shared state in the registry. If
there is already a state
- * handle registered under the given key, the "new" state handle is
disposed .
+ * Register a reference to the given shared state in the registry, the
registry key should be
+ * based on the physical identification of the state. If there is already
a state handle
+ * registered under the given key and the 'new' state is not equal to the
old one, an exception
Review Comment:
NIT:
```suggestion
* Register a reference to the given shared state in the registry. The
registry key should be
* based on the physical identifier of the state. If there is already a
state handle
* registered under the same key and the 'new' state is not equal to the
old one, an exception
```
##########
flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java:
##########
@@ -54,19 +54,21 @@ default StreamStateHandle registerReference(
}
/**
- * Register a reference to the given shared state in the registry. If
there is already a state
- * handle registered under the given key, the "new" state handle is
disposed .
+ * Register a reference to the given shared state in the registry, the
registry key should be
+ * based on the physical identification of the state. If there is already
a state handle
+ * registered under the given key and the 'new' state is not equal to the
old one, an exception
+ * will be thrown.
*
- * <p>IMPORTANT: caller should check the state handle returned by the
result, because the
- * registry is performing de-duplication and could potentially return a
handle that is supposed
- * to replace the one from the registration request.
+ * <p>IMPORTANT: After FLINK-29913, caller only needs to check the state
handle returned by the
+ * result only if the 'new' state is a placeholder, because the registry
is no longer performing
+ * de-duplication.
*
* @param state the shared state for which we register a reference.
* @param checkpointID which uses the state
* @param preventDiscardingCreatedCheckpoint as long as this state is
still in use. The
* "checkpoint that created the state" is recorded on the first state
registration.
- * @return the result of this registration request, consisting of the
state handle that is
- * registered under the key by the end of the operation and its
current reference count.
+ * @return the result of this registration request, it will differ from
the passed one if the
+ * 'new' state is a placeholder.
Review Comment:
```suggestion
* @return the state handle registered under the given key. It might
differ from the passed state handle, e.g. if it was a placeholder.
```
##########
flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java:
##########
@@ -54,19 +54,21 @@ default StreamStateHandle registerReference(
}
/**
- * Register a reference to the given shared state in the registry. If
there is already a state
- * handle registered under the given key, the "new" state handle is
disposed .
+ * Register a reference to the given shared state in the registry, the
registry key should be
+ * based on the physical identification of the state. If there is already
a state handle
+ * registered under the given key and the 'new' state is not equal to the
old one, an exception
+ * will be thrown.
*
- * <p>IMPORTANT: caller should check the state handle returned by the
result, because the
- * registry is performing de-duplication and could potentially return a
handle that is supposed
- * to replace the one from the registration request.
+ * <p>IMPORTANT: After FLINK-29913, caller only needs to check the state
handle returned by the
+ * result only if the 'new' state is a placeholder, because the registry
is no longer performing
+ * de-duplication.
Review Comment:
I think this is an implementation detail about the handle types.
I'd rather suggest to always check the result, so that we don't need to
update callers if a new replacement is introduced:
```suggestion
* <p>IMPORTANT: the caller must use the returned state handle instead
of the passed one
* because the registry might replace or update it.
```
--
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]
