liyafan82 commented on a change in pull request #8236:
[FLINK-12289][flink-runtime]Fix bugs and typos in Memory manager
URL: https://github.com/apache/flink/pull/8236#discussion_r277931471
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/memory/MemoryManager.java
##########
@@ -387,11 +387,11 @@ public void release(MemorySegment segment) {
*
* @param segments The segments to be released.
* @throws NullPointerException Thrown, if the given collection is null.
- * @throws IllegalArgumentException Thrown, id the segments are of an
incompatible type.
+ * @throws IllegalArgumentException Thrown, if the segments are of an
incompatible type.
*/
public void release(Collection<MemorySegment> segments) {
if (segments == null) {
- return;
+ throw new NullPointerException();
Review comment:
> If we get a `null` to release, why can't we just skip it? I think it's a
defensive code not a bug. Or can you post some bad situation about this?
Hi Aitozi, thanks a lot for your comment. The JavaDoc for this method
declares "@throws NullPointerException Thrown, if the given collection is
null." So we should either throw an NPE, or change the JavaDoc, to make it
consistent.
By referencing other methods in this class, I think it is preferable to make
the method throw NPE, because the methods should follow a consistent style.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
