PIP-17: provide DataBlockHeader and implementation #1669

zhaijack · 2018-04-27T13:01:30Z

Motivation

Provide DataBlockHeader implementation. A block header is in format:
[ magic_word -- int ][ block_len -- int ][ first_entry_id -- long][alignment padding]

Master issue #1511

Modifications

Add class DataBlockHeader and test for it.

Result

unit test pass.

sijie · 2018-04-27T17:21:45Z

@ivankelly please review this

ivankelly · 2018-04-30T09:16:55Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/impl/DataBlockHeaderImpl.java

+     * Read out in format:
+     *   [ magic_word -- int ][ block_len -- int ][ first_entry_id  -- long]
+     */
+    public InputStream toStream() throws IOException {


nit: @OverRide

ivankelly · 2018-04-30T09:17:20Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/impl/DataBlockHeaderImpl.java

+    }
+
+    // Construct DataBlockHeader from InputStream
+    public static DataBlockHeaderImpl fromStream(InputStream stream) throws IOException {


return DataBlockHeader, rather than the impl

ivankelly · 2018-04-30T09:20:21Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/DataBlockHeader.java

+    /**
+     * Get the size of this DataBlockHeader.
+     */
+    int getHeaderSize();


How is header size useful to a user of the DataBlockHeader? I think this should return what getDataStartOffset is returning below. The contents of the header should be opaque to the user of the interface.

Thanks. Seems this is the key for some of the comments below. If user get the actual size, user could ignore the paddings, InputStream passed into 'fromStream', and returned by 'toStream' could ignore the paddings.
will change this to what you suggested.

I don't think getHeaderSize() is needed anymore.

ivankelly · 2018-04-30T09:20:50Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/DataBlockHeader.java

+     * Get Magic Word for data block.
+     * It is a sequence of bytes used to identify the start of a block.
+     */
+    int getBlockMagicWord();


Is this useful externally? Can be static in any case.

ivankelly · 2018-04-30T09:23:12Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/impl/DataBlockHeaderImpl.java

+        DataInputStream dis = new DataInputStream(stream);
+        int magic = dis.readInt();
+        checkState(magic == dataBlockMagicWord);
+        return new DataBlockHeaderImpl(dis.readInt(), dis.readLong());


When reading the header from a stream, you want to move the input stream read cursor past the padding also. The user of data block header should not be aware that there is padding, and that the data only takes up part of the header. Instead, they should pass the turn a header into an input stream and get a stream that returns dataBlockHeaderAlign bytes. They should pass an input stream into this method, and this method should read dataBlockHeaderAlign bytes and return a DataBlockHeader object, even if only 4 of the bytes read are useful for building the DataBlockHeader object.

ivankelly · 2018-04-30T09:41:04Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/impl/DataBlockHeaderImpl.java

+    public static DataBlockHeaderImpl fromStream(InputStream stream) throws IOException {
+        DataInputStream dis = new DataInputStream(stream);
+        int magic = dis.readInt();
+        checkState(magic == dataBlockMagicWord);


checkState throws an unchecked IllegalStateException. Since this is reading external data, we should throw an IOException if the magic number is wrong, or at least a checked exception, so that it can be handled more gracefully by the caller.

ivankelly · 2018-04-30T09:43:19Z

pulsar-broker/src/test/java/org/apache/pulsar/s3offload/DataBlockHeaderTest.java

+        assertTrue(rebuild.getBlockLength() == headerLength);
+        assertTrue(rebuild.getFirstEntryId() == firstEntryId);
+    }
+


Test what happens when you read a header that's too small

Test what happens when you read complete junk.

Test reading to the end of the stream returned by toStream(), ensuring that the number of bytes read is correct.

ivankelly · 2018-04-30T09:52:13Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/impl/DataBlockHeaderImpl.java

+            .writeInt(blockLength)
+            .writeLong(firstEntryId);
+
+        return new ByteBufInputStream(out, true);


Add a comment that the true argument mean that the inputstream will release the bytebuf on close.

zhaijack · 2018-04-30T13:03:32Z

@ivankelly updated following your comments.

ivankelly · 2018-04-30T13:19:39Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/impl/DataBlockHeaderImpl.java

+        DataInputStream dis = new DataInputStream(stream);
+        int magic = dis.readInt();
+        if (magic != dataBlockMagicWord) {
+            throw new IOException("Data block header magic word not match.");


include what was expected and what was read in the error message.

ivankelly · 2018-04-30T13:21:07Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/impl/DataBlockHeaderImpl.java

+        // verify padding
+        for (int index = 0; index < dataBlockHeaderAlign - dataBlockHeaderSize; index ++) {
+            if (dis.readByte() != padding) {
+                throw new IOException("Data block header magic word not match.");


I wouldn't validate that all the padding is 0. This could break BC if we add a field, to the header, but then an older version of the software tried to read it.

this can be dis.skipBytes(dataBlockHeaderAlign - dataBlockHeaderSize);

ivankelly · 2018-04-30T13:23:30Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/impl/DataBlockHeaderImpl.java

+    private static final int dataBlockMagicWord = 0xDBDBDBDB;
+    // This is bigger than header size. Leaving some place for alignment and future enhancement.
+    // Payload use this as the start offset.
+    private static final int dataBlockHeaderAlign = 128;


Rename this to HEADER_MAX_SIZE

ivankelly · 2018-04-30T13:25:31Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/impl/DataBlockHeaderImpl.java

+public class DataBlockHeaderImpl implements DataBlockHeader {
+    // Magic Word for data block.
+    // It is a sequence of bytes used to identify the start of a block.
+    private static final int dataBlockMagicWord = 0xDBDBDBDB;


dataBlock is redundant in these names. also, oracle java coding standard states that constants should be UPPER_CASE with underscores, so this should be MAGIC_WORD.

ivankelly · 2018-04-30T13:27:24Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/impl/DataBlockHeaderImpl.java

+        out.writeInt(dataBlockMagicWord)
+            .writeInt(blockLength)
+            .writeLong(firstEntryId)
+            .writeBytes(new byte[dataBlockHeaderAlign - headerSize]);


the padding will always be the same size, so define PADDING = new byte[x] as a constant in the class. When reading back, you can skip PADDING.length also.

ivankelly · 2018-04-30T13:28:11Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/DataBlockHeader.java

+     * Get Magic Word for data block.
+     * It is a sequence of bytes used to identify the start of a block.
+     */
+    static int getBlockMagicWord() {


remove from interface. external callers don't need it.

ivankelly · 2018-04-30T13:30:03Z

pulsar-broker/src/test/java/org/apache/pulsar/s3offload/DataBlockHeaderTest.java

+        stream.reset();
+        byte streamContent[] = new byte[DataBlockHeader.getDataStartOffset()];
+
+        // stream with all 0, simulate junk data, should throw exception for header magic not match.


Make these a separate test case. each test case should test one thing.

ivankelly · 2018-04-30T13:32:25Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/DataBlockHeader.java

+     * Get the payload start offset in this block.
+     * Space before this offset is for header and alignment.
+     */
+    static int getDataStartOffset() {


Remove from interface. If tests need this value, they can get it from the Impl.

ivankelly · 2018-04-30T13:33:46Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/DataBlockHeader.java

+    /**
+     * Get the size of this DataBlockHeader.
+     */
+    int getHeaderSize();


I don't think getHeaderSize() is needed anymore.

ivankelly · 2018-04-30T13:35:46Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/s3offload/DataBlockHeader.java

+public interface DataBlockHeader {
+
+    /**
+     * Get the length of the block in bytes, including the header.


Should this size include the header? If I have an inputstream, and im reading in, it would seem more natural, that after I've read in the header, the block length that I have is just the block length of the payload.

Thanks. This is following the design doc, It would be helpful updating the doc if the idea changed.
And it seems more natural to me to make blockLength means the whole block length as in the design doc, since header and payload stored in the same block.

Ok. Lets leave it for now, and change it if we need to when we use it.

ivankelly · 2018-04-30T14:45:27Z

@zhaijack lgtm. Will you add the master issue to the PR description, so it gets linked there.

zhaijack · 2018-04-30T14:56:04Z

Thanks @ivankelly , added it.

sijie · 2018-04-30T17:17:14Z

moved this to 2.1

ivankelly · 2018-05-02T09:04:08Z

Weird. It looks like the test hangs on MessageIdTest for over 2 hours. Obviously nothing to do with this change though.

sijie · 2018-05-02T17:04:27Z

retest this please

…1678) ### Motivation Implementation of InputStream(BlockAwareSegmentInputStream). when calls the is.read(), it return the following data: - Block header - Ledger entries starting from passed in entryId - Padding in the with bytes 0xFEDCDEAD This is based on PR #1669 (commit 3c8a846), And PR #1669 should be reviewed and merged first. Master issue #1511 ### Modifications Add class BlockAwareSegmentInputStream and test for it. ### Result unit test pass.

add DataBlockHeader

3c8a846

sijie approved these changes Apr 27, 2018

View reviewed changes

sijie assigned zhaijack Apr 27, 2018

sijie added the area/tieredstorage label Apr 27, 2018

sijie added this to the 2.0.0-incubating milestone Apr 27, 2018

zhaijack mentioned this pull request Apr 28, 2018

PIP-17: provide BlockAwareSegmentInputStream implementation and test #1678

Merged

ivankelly requested changes Apr 30, 2018

View reviewed changes

ivankelly reviewed Apr 30, 2018

View reviewed changes

change following @ivan's comments

88f3746

ivankelly requested changes Apr 30, 2018

View reviewed changes

ivankelly approved these changes Apr 30, 2018

View reviewed changes

sijie modified the milestones: 2.0.0-incubating, 2.1.0-incubating Apr 30, 2018

change following @ivan's comments

f0b06f7

zhaijack force-pushed the s3-block-header branch from 9724e98 to f0b06f7 Compare May 1, 2018 06:40

sijie merged commit d1e719c into apache:master May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP-17: provide DataBlockHeader and implementation #1669

PIP-17: provide DataBlockHeader and implementation #1669

zhaijack commented Apr 27, 2018 •

edited

sijie commented Apr 27, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

zhaijack Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

zhaijack commented Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

ivankelly Apr 30, 2018

zhaijack Apr 30, 2018 •

edited

ivankelly Apr 30, 2018

ivankelly commented Apr 30, 2018

zhaijack commented Apr 30, 2018

sijie commented Apr 30, 2018

ivankelly commented May 2, 2018 •

edited

sijie commented May 2, 2018

PIP-17: provide DataBlockHeader and implementation #1669

PIP-17: provide DataBlockHeader and implementation #1669

Conversation

zhaijack commented Apr 27, 2018 • edited

Motivation

Modifications

Result

sijie commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaijack commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaijack Apr 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankelly commented Apr 30, 2018

zhaijack commented Apr 30, 2018

sijie commented Apr 30, 2018

ivankelly commented May 2, 2018 • edited

sijie commented May 2, 2018

zhaijack commented Apr 27, 2018 •

edited

zhaijack Apr 30, 2018 •

edited

ivankelly commented May 2, 2018 •

edited