[jira] [Commented] (FLINK-7515) allow actual 0-length content in NettyMessage#allocateBuffer()

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-7515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16240431#comment-16240431
 ] 

ASF GitHub Bot commented on FLINK-7515:
---

Github user asfgit closed the pull request at:

https://github.com/apache/flink/pull/4592


> allow actual 0-length content in NettyMessage#allocateBuffer()
> --
>
> Key: FLINK-7515
> URL: https://issues.apache.org/jira/browse/FLINK-7515
> Project: Flink
>  Issue Type: Sub-task
>  Components: Network
>Affects Versions: 1.4.0
>Reporter: Nico Kruber
>Assignee: Nico Kruber
>Priority: Minor
>
> Previously, length {{0}} meant "unknown content length" but there are cases 
> where the actual length is 0 and we do not need a larger buffer. Let's use 
> {{-1}} for tagging the special case instead.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (FLINK-7515) allow actual 0-length content in NettyMessage#allocateBuffer()

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-7515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16240281#comment-16240281
 ] 

ASF GitHub Bot commented on FLINK-7515:
---

Github user tillrohrmann commented on a diff in the pull request:

https://github.com/apache/flink/pull/4592#discussion_r149077193
  
--- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/netty/NettyMessage.java
 ---
@@ -67,12 +68,53 @@
 
// 

 
+   /**
+* Allocates a new (header and contents) buffer and adds some header 
information for the frame
--- End diff --

This method only allocates memory for the header and no contents (or 
contents is empty). Maybe we could update the JavaDoc accordingly.


> allow actual 0-length content in NettyMessage#allocateBuffer()
> --
>
> Key: FLINK-7515
> URL: https://issues.apache.org/jira/browse/FLINK-7515
> Project: Flink
>  Issue Type: Sub-task
>  Components: Network
>Affects Versions: 1.4.0
>Reporter: Nico Kruber
>Assignee: Nico Kruber
>Priority: Minor
>
> Previously, length {{0}} meant "unknown content length" but there are cases 
> where the actual length is 0 and we do not need a larger buffer. Let's use 
> {{-1}} for tagging the special case instead.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (FLINK-7515) allow actual 0-length content in NettyMessage#allocateBuffer()

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-7515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16233887#comment-16233887
 ] 

ASF GitHub Bot commented on FLINK-7515:
---

Github user NicoK commented on the issue:

https://github.com/apache/flink/pull/4592
  
of course, the last change added a conflict...rebased now


> allow actual 0-length content in NettyMessage#allocateBuffer()
> --
>
> Key: FLINK-7515
> URL: https://issues.apache.org/jira/browse/FLINK-7515
> Project: Flink
>  Issue Type: Sub-task
>  Components: Network
>Affects Versions: 1.4.0
>Reporter: Nico Kruber
>Assignee: Nico Kruber
>Priority: Minor
>
> Previously, length {{0}} meant "unknown content length" but there are cases 
> where the actual length is 0 and we do not need a larger buffer. Let's use 
> {{-1}} for tagging the special case instead.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (FLINK-7515) allow actual 0-length content in NettyMessage#allocateBuffer()

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-7515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16233880#comment-16233880
 ] 

ASF GitHub Bot commented on FLINK-7515:
---

Github user pnowojski commented on the issue:

https://github.com/apache/flink/pull/4592
  
LGTM (besides "Conflicting files")


> allow actual 0-length content in NettyMessage#allocateBuffer()
> --
>
> Key: FLINK-7515
> URL: https://issues.apache.org/jira/browse/FLINK-7515
> Project: Flink
>  Issue Type: Sub-task
>  Components: Network
>Affects Versions: 1.4.0
>Reporter: Nico Kruber
>Assignee: Nico Kruber
>Priority: Minor
>
> Previously, length {{0}} meant "unknown content length" but there are cases 
> where the actual length is 0 and we do not need a larger buffer. Let's use 
> {{-1}} for tagging the special case instead.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (FLINK-7515) allow actual 0-length content in NettyMessage#allocateBuffer()

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-7515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16233816#comment-16233816
 ] 

ASF GitHub Bot commented on FLINK-7515:
---

Github user NicoK commented on a diff in the pull request:

https://github.com/apache/flink/pull/4592#discussion_r148212023
  
--- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/netty/NettyMessage.java
 ---
@@ -64,12 +65,53 @@
 
// 

 
+   /**
+* Allocates a new (header and contents) buffer and adds some header 
information for the frame
+* decoder.
+*
+* Before sending the buffer, you must write the actual length after 
adding the contents as
+* an integer to position 0!
+*
+* @param allocator
+*  byte buffer allocator to use
+* @param id
+*  {@link NettyMessage} subclass ID
+*
+* @return a newly allocated direct buffer with header data written for 
{@link
+* NettyMessageDecoder}
+*/
private static ByteBuf allocateBuffer(ByteBufAllocator allocator, byte 
id) {
-   return allocateBuffer(allocator, id, 0);
+   return allocateBuffer(allocator, id, -1);
}
 
+   /**
+* Allocates a new (header and contents) buffer and adds some header 
information for the frame
+* decoder.
+*
+* If the length is unknown, you must write the actual 
length after adding the
+* contents as an integer to position 0!
+*
+* @param allocator
+*  byte buffer allocator to use
+* @param id
+*  {@link NettyMessage} subclass ID
+* @param length
+*  content length (or -1 if unknown)
+*
+* @return a newly allocated direct buffer with header data written for 
{@link
+* NettyMessageDecoder}
+*/
private static ByteBuf allocateBuffer(ByteBufAllocator allocator, byte 
id, int length) {
-   final ByteBuf buffer = length != 0 ? 
allocator.directBuffer(HEADER_LENGTH + length) : allocator.directBuffer();
+   Preconditions.checkArgument(length <= Integer.MAX_VALUE - 
HEADER_LENGTH);
--- End diff --

why not...


> allow actual 0-length content in NettyMessage#allocateBuffer()
> --
>
> Key: FLINK-7515
> URL: https://issues.apache.org/jira/browse/FLINK-7515
> Project: Flink
>  Issue Type: Sub-task
>  Components: Network
>Affects Versions: 1.4.0
>Reporter: Nico Kruber
>Assignee: Nico Kruber
>Priority: Minor
>
> Previously, length {{0}} meant "unknown content length" but there are cases 
> where the actual length is 0 and we do not need a larger buffer. Let's use 
> {{-1}} for tagging the special case instead.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (FLINK-7515) allow actual 0-length content in NettyMessage#allocateBuffer()

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-7515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16233771#comment-16233771
 ] 

ASF GitHub Bot commented on FLINK-7515:
---

Github user pnowojski commented on a diff in the pull request:

https://github.com/apache/flink/pull/4592#discussion_r148203203
  
--- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/netty/NettyMessage.java
 ---
@@ -64,12 +65,53 @@
 
// 

 
+   /**
+* Allocates a new (header and contents) buffer and adds some header 
information for the frame
+* decoder.
+*
+* Before sending the buffer, you must write the actual length after 
adding the contents as
+* an integer to position 0!
+*
+* @param allocator
+*  byte buffer allocator to use
+* @param id
+*  {@link NettyMessage} subclass ID
+*
+* @return a newly allocated direct buffer with header data written for 
{@link
+* NettyMessageDecoder}
+*/
private static ByteBuf allocateBuffer(ByteBufAllocator allocator, byte 
id) {
-   return allocateBuffer(allocator, id, 0);
+   return allocateBuffer(allocator, id, -1);
}
 
+   /**
+* Allocates a new (header and contents) buffer and adds some header 
information for the frame
+* decoder.
+*
+* If the length is unknown, you must write the actual 
length after adding the
+* contents as an integer to position 0!
+*
+* @param allocator
+*  byte buffer allocator to use
+* @param id
+*  {@link NettyMessage} subclass ID
+* @param length
+*  content length (or -1 if unknown)
+*
+* @return a newly allocated direct buffer with header data written for 
{@link
+* NettyMessageDecoder}
+*/
private static ByteBuf allocateBuffer(ByteBufAllocator allocator, byte 
id, int length) {
-   final ByteBuf buffer = length != 0 ? 
allocator.directBuffer(HEADER_LENGTH + length) : allocator.directBuffer();
+   Preconditions.checkArgument(length <= Integer.MAX_VALUE - 
HEADER_LENGTH);
--- End diff --

import static?


> allow actual 0-length content in NettyMessage#allocateBuffer()
> --
>
> Key: FLINK-7515
> URL: https://issues.apache.org/jira/browse/FLINK-7515
> Project: Flink
>  Issue Type: Sub-task
>  Components: Network
>Affects Versions: 1.4.0
>Reporter: Nico Kruber
>Assignee: Nico Kruber
>Priority: Minor
>
> Previously, length {{0}} meant "unknown content length" but there are cases 
> where the actual length is 0 and we do not need a larger buffer. Let's use 
> {{-1}} for tagging the special case instead.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (FLINK-7515) allow actual 0-length content in NettyMessage#allocateBuffer()

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-7515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16141553#comment-16141553
 ] 

ASF GitHub Bot commented on FLINK-7515:
---

GitHub user NicoK opened a pull request:

https://github.com/apache/flink/pull/4592

[FLINK-7515][network] allow actual 0-length content in 
NettyMessage#allocateBuffer()

## What is the purpose of the change

Previously, length "0" meant "unknown content length" but there are cases 
where the actual length is "0" and we do not need a larger buffer than the 
header size.

## Brief change log

- use -1 for tagging the special case of an unknown content length
- change buffers with 0-size content to start with the header size only

## Verifying this change

This change is already covered by existing tests, such as 
`NettyMessageSerializationTest` and many more network-related tests.

## Does this pull request potentially affect one of the following parts:

  - Dependencies (does it add or upgrade a dependency): (no)
  - The public API, i.e., is any changed class annotated with 
`@Public(Evolving)`: (no)
  - The serializers: (no)
  - The runtime per-record code paths (performance sensitive): (yes)
  - Anything that affects deployment or recovery: JobManager (and its 
components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)

## Documentation

  - Does this pull request introduce a new feature? (no)
  - If yes, how is the feature documented? (JavaDocs)


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/NicoK/flink flink-7515

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/4592.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #4592


commit 828548d6e4b2390fc607f55382aa1af2ffa5095f
Author: Nico Kruber 
Date:   2017-08-24T15:14:38Z

[FLINK-7515][network] allow actual 0-length content in 
NettyMessage#allocateBuffer()

Previously, length "0" meant "unknown content length" but there are cases 
where
the actual length is 0 and so we use -1 for tagging the special case now.




> allow actual 0-length content in NettyMessage#allocateBuffer()
> --
>
> Key: FLINK-7515
> URL: https://issues.apache.org/jira/browse/FLINK-7515
> Project: Flink
>  Issue Type: Sub-task
>  Components: Network
>Affects Versions: 1.4.0
>Reporter: Nico Kruber
>Assignee: Nico Kruber
>Priority: Minor
>
> Previously, length {{0}} meant "unknown content length" but there are cases 
> where the actual length is 0 and we do not need a larger buffer. Let's use 
> {{-1}} for tagging the special case instead.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)