hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ramkrishna vasudevan <ramkrishna.s.vasude...@gmail.com>
Subject Re: ByteBuffer Backed Cell - New APIs (HBASE-12358)
Date Sat, 06 Dec 2014 17:36:26 GMT
Thanks for valuable feedback from every one.
>>Is there discussion on why ByteRange and netty ByteBuf were by-passed (I
have a good notion of why but would be good to have this in a 'design/spec'
doc for completeness since they were contenders)

True. I would leave to Anoop to comment on that. But the reason is the RPC
layer and the socket layer that accepts only BBs.

>>Is this enough?  If offheap, to do compares, won't we also need positional
gets?

Yes it would be enough. Even if we have offheap buffer we could do reads
with Unsafe comparators - for absolute gets on the BB.
And surely extending the Cell would make as add for comparisons and 'if'
checks.

>>a new object -- either
each time a getXXXBuffer is called or a Cell instance will cache these
slices so Cells will grow at least a reference * each Cell part.

In (1) we won't do slice. So there is no new object at all. It would be the
same buffer that we created from the HFileBlock that is denoting a Cell.

Slicing is as in (2). But if we slice the buffer and create new object -
the suggestion of caching it (as a ref) makes sense.
But still (2) would mean that in cases of blockSeeks we end up in creating
lot of small objects and as Lars says it would have a JVM reference to
clean those small objects.

>>What if we did not add getXXXBuffer to the API?  A Cell could be backed by
an onheap BB without having to reveal this fact in the API?  If offheap BB,
we'd need to copy the Cell Key pieces onheap to support getArray. How bad
would that be?

Stack - if we don't add getXXXBuffer and try to use getArray() then we lose
the whole purpose of supporting read path directly from the BB underlying
in block cache. May be for onheap BB it is fine but I think we should have
some generic way to access onheap and offheap BB thus abstracting that
information from the user.


>>Yeah. This is broke. You can't have getXXXBuffer being offheap one-minute,
then on-heap the next.
Yes - that is why we wanted to have a proper contract on this.  Jon'
suggestion of adding getBBOFfset and getArrayOffset would solve that but
would add more confusion.

>>We'd purge getXXXLength and getXXXOffset if we go route #2.
Yes we have to.  Or if we don't purge then getXXXLength and getXXXOffset
would be used only with getXXXArray and with getXXXBuffer no need of those
APIs.

>>So returning a reference to an existing BB (on the stack) along with
offset and length is better from that angle.
This is myy opinion too. And I would go with this option.

>>KeyValue.getBuffer(). We have not even finished that part, yet.
Lars - I think this is done both in read path and write path.



On Sat, Dec 6, 2014 at 12:25 PM, lars hofhansl <larsh@apache.org> wrote:

> A slice make a new object, though. We'll inundate in the JVM with lots and
> lots of small objects that it needs to trace.So returning a reference to an
> existing BB (on the stack) along with offset and length is better from that
> angle.
> Also, before we do any of that we need to get rid of all occurrences of
> KeyValue.getBuffer(). We have not even finished that part, yet.
> -- Lars
>       From: Nick Dimiduk <ndimiduk@gmail.com>
>  To: "dev@hbase.apache.org" <dev@hbase.apache.org>
>  Sent: Friday, December 5, 2014 8:33 AM
>  Subject: Re: ByteBuffer Backed Cell - New APIs (HBASE-12358)
>
> From the perspective of API going forward, I prefer (2). BB already allows
> for encapsulating a sequence if bytes in a single object, so why
> re-implement that? I suspect you'll want to return slices to the caller
> anyway, because you don't want them messing with the backing store's
> position,
> limit, etc state.
>
> On Friday, December 5, 2014, Jonathan Hsieh <jon@cloudera.com> wrote:
>
> > I think of it from a compatibility point of view - each method has its
> own
> > semantics and the semantics should not morph between versions.
> >
> > With the repurposing the old apis approach, someone who ports to the new
> > version without porting to the api will likely run into some nasty
> > surprises.  Worse yet, they'd need to modify their code and understand
> the
> > details of what the semantics changes are.
> >
> > Having completely new apis makes it easier to see when deprecated
> > behavior/methods are removed and doesn't require code inspection to
> verify.
> > (can rely on compiler warnings)  Also, old code won't be broken (just
> less
> > efficient).
> >
> > Jon.
> >
> >
> >
> > On Thu, Dec 4, 2014 at 10:41 PM, ramkrishna vasudevan <
> > ramkrishna.s.vasudevan@gmail.com <javascript:;>> wrote:
> >
> > > We thought about this suggestion too but this comes with a lot of more
> > > confusion (?) - what do you think?
> > > Adding new APIs but similar in behaviour could lead to lot more
> > confusion?
> > > Internally it is more easy.
> > >
> > >
> > > On Fri, Dec 5, 2014 at 11:10 AM, Jonathan Hsieh <jon@cloudera.com
> > <javascript:;>> wrote:
> > >
> > > > Ram,
> > > >
> > > > Can we essentially do both by creating the new getXxBuffer, and then
> > also
> > > > creating new offset and len apis -- getXxx(Bb|Buffer|Buf|B)Offset and
> > > > getXxx(Bb|Buffer|Buf|B)Length?
> > > >
> > > > The old getXxxArray could use the old getXxxOffset and getXxxLength
> > > calls.
> > > > Also we'd deprecate all of these and provide an sliced and diced
> > version
> > > > that would have offset always 0.
> > > >
> > > > This way we wouldn't conflate the Bb and byte[] offsets and lengths.
> > > Also
> > > > we could behind the scenes convert Bbs to byte[] arrays and convert
> > > > byte[]'s in to Bbs while maintaining the same interface.
> > > >
> > > > Jon.
> > > >
> > > >
> > > >
> > > > On Thu, Dec 4, 2014 at 7:24 PM, ramkrishna vasudevan <
> > > > ramkrishna.s.vasudevan@gmail.com <javascript:;>> wrote:
> > > >
> > > > > Hi Devs
> > > > >
> > > > > This write up is to provide a brief idea  on why we need a BB
> backed
> > > cell
> > > > > and what are the items that we need to take care before introducing
> > new
> > > > > APIs in Cell that are BB backed.
> > > > >
> > > > > Pls refer to https://issues.apache.org/jira/browse/HBASE-12358
> also
> > > and
> > > > > its
> > > > > parent JIRA https://issues.apache.org/jira/browse/HBASE-11425 for
> > the
> > > > > history.
> > > > >
> > > > > Coming back to the discussion on new APIs, this discussion is based
> > on
> > > > > supporting BB in the read path (write path is not targeted now) so
> > that
> > > > we
> > > > > could work with offheap BBs also. This would avoid copying of data
> > from
> > > > > BlockCache to the read path ByteBuffer.
> > > > >
> > > > > Assume we will be working with BBs in the read path, We will need
> to
> > > > >  introduce *getXXXBuffer() *APIs and also *hasArray()* in Cell
> itself
> > > > > directly.
> > > > > If we try to extend the cell or create a new Cell then *everywhere
> we
> > > > need
> > > > > to do instanceOf check or do type conversion *and that is why
> adding
> > > new
> > > > > APIs to Cell interface itself makes sense.
> > > > >
> > > > > Plan is to use this *getXXXBuffer()* API through out the read path
> > > > *instead
> > > > > of getXXXArray()*.
> > > > >
> > > > > Now there are two ways to use it
> > > > >
> > > > > 1) Use getXXXBuffer() along with getXXXOffset(), getXXXLength()
> like
> > > how
> > > > we
> > > > > use now for getXXXArray() APIs with the offset and length. Doing
so
> > > would
> > > > > ensure that every where in the filters and CP one has to just
> replace
> > > the
> > > > > getXXXArray() with getXXXBuffer() and continue to use
> getXXXOffset()
> > > and
> > > > > getXXXLength(). We would do some wrapping of the byte[] with a BB
> > > incase
> > > > of
> > > > > KeyValue type of cells so that getXXXBuffer along with offset and
> > > length
> > > > > holds true everywhere. Note that here if hasArray is true(for KV
> > case)
> > > > then
> > > > > getXXXArray() would also work.
> > > > >
> > > > > 2)The other way of using this is that use only getXXXBuffer() API
> and
> > > > > ensure that the BB is always duplicated/sliced and only the portion
> > of
> > > > the
> > > > > total BB is returned which represents the individual component of
> the
> > > > Cell.
> > > > > In this case there is no use of getXXXOffset() (as it is going to
> be
> > 0)
> > > > and
> > > > > getXXXLength() is any way going to be the sliced BB's limit.
> > > > >
> > > > > But in the 2nd approach we may end up in creating lot of small
> > objects
> > > > even
> > > > > while doing comparison.
> > > > >
> > > > > Now the next problem that comes is what to do with the
> getXXXArray()
> > > > APIs.
> > > > > If one sees hasArray() as false (a DBB backed Cell) and uses the
> > > > > getXXXArray() API along with offset and length - what should we do.
> > > > Should
> > > > > we create a byte[] from the DBB and return it? Then in that case
> what
> > > > would
> > > > > should the *getXXXOffset() return for a getXXXBuffer or
> > getXXXArray()?*
> > > > >
> > > > > If we go with the 2nd approach then getXXXBuffer() should be
> clearly
> > > > > documented saying that it has to be used without getXXXOffset() and
> > > > > getXXXLength() and use getXXXOffset() and getXXXLength() only with
> > > > > getXXXArray().
> > > > >
> > > > > Now if a Cell is backed by on heap BB then we could definitely
> return
> > > > > getXXXArray() also - but what to return in the getXXXOffset() would
> > be
> > > > > determined by what approach to use for getXXXBuffer(). (based on
> (1)
> > > and
> > > > > (2)).
> > > > >
> > > > > We wanted to open up this topic now so that to get some feedback
on
> > > what
> > > > > could be an option here. Since it is an user facing Interface we
> need
> > > to
> > > > be
> > > > > careful with this.
> > > > >
> > > > > I would suggest that whenever a Cell is *BB backed*(Onheap or
> > offheap)
> > > > > always *hasArray() would be false* in that Cell impl.
> > > > >
> > > > > Every where we would use getXXXBuffer() along with getXXXOffest()
> and
> > > > > getXXXLength(). Even in case of KV we could wrap the byte[] with
BB
> > so
> > > > that
> > > > > we have uniformity through the read code and we don't have too many
> > > 'if'
> > > > > else conditions.
> > > > >
> > > > > When ever *hasArray() is false* - using getXXXArray() API would
> throw
> > > > > *UnSupportedOperation
> > > > > Exception*.
> > > > >
> > > > > As said if we want *getXXXArray()* to be supported as per the
> > existing
> > > > way
> > > > > then getXXXBuffer() and getXXXOffset(), getXXXLength() should be
> > > clearly
> > > > > documented.
> > > > >
> > > > > Thoughts!!!
> > > > >
> > > > > Regards
> > > > > Ram & Anoop
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > // Jonathan Hsieh (shay)
> > > > // HBase Tech Lead, Software Engineer, Cloudera
> > > > // jon@cloudera.com <javascript:;> // @jmhsieh
>
>
> > > >
> > >
> >
> >
> >
> > --
> > // Jonathan Hsieh (shay)
> > // HBase Tech Lead, Software Engineer, Cloudera
> > // jon@cloudera.com <javascript:;> // @jmhsieh
> >
>
>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message