You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by NicoK <gi...@git.apache.org> on 2017/08/25 12:36:52 UTC
[GitHub] flink pull request #4592: [FLINK-7515][network] allow actual 0-length conten...
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 <ni...@data-artisans.com>
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.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4592: [FLINK-7515][network] allow actual 0-length content in Ne...
Posted by NicoK <gi...@git.apache.org>.
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/4592
of course, the last change added a conflict...rebased now
---
[GitHub] flink pull request #4592: [FLINK-7515][network] allow actual 0-length conten...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4592
---
[GitHub] flink pull request #4592: [FLINK-7515][network] allow actual 0-length conten...
Posted by NicoK <gi...@git.apache.org>.
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.
+ *
+ * <p>Before sending the buffer, you must write the actual length after adding the contents as
+ * an integer to position <tt>0</tt>!
+ *
+ * @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.
+ *
+ * <p>If the <tt>length</tt> is unknown, you must write the actual length after adding the
+ * contents as an integer to position <tt>0</tt>!
+ *
+ * @param allocator
+ * byte buffer allocator to use
+ * @param id
+ * {@link NettyMessage} subclass ID
+ * @param length
+ * content length (or <tt>-1</tt> 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...
---
[GitHub] flink pull request #4592: [FLINK-7515][network] allow actual 0-length conten...
Posted by pnowojski <gi...@git.apache.org>.
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.
+ *
+ * <p>Before sending the buffer, you must write the actual length after adding the contents as
+ * an integer to position <tt>0</tt>!
+ *
+ * @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.
+ *
+ * <p>If the <tt>length</tt> is unknown, you must write the actual length after adding the
+ * contents as an integer to position <tt>0</tt>!
+ *
+ * @param allocator
+ * byte buffer allocator to use
+ * @param id
+ * {@link NettyMessage} subclass ID
+ * @param length
+ * content length (or <tt>-1</tt> 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?
---
[GitHub] flink issue #4592: [FLINK-7515][network] allow actual 0-length content in Ne...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on the issue:
https://github.com/apache/flink/pull/4592
LGTM (besides "Conflicting files")
---
[GitHub] flink pull request #4592: [FLINK-7515][network] allow actual 0-length conten...
Posted by tillrohrmann <gi...@git.apache.org>.
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.
---