Mailing-List: contact dev-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hbase.apache.org
Received-SPF: neutral (athena.apache.org: 184.73.217.71 is neither permitted
 nor denied by domain of pranavkhaitan@gmail.com)
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Subject: Re: Review Request: delete followed by a put with the same timestamp
From: "Pranav Khaitan" <pranavkhaitan@gmail.com>
To: "Kannan Muthukkaruppan" <kannan@facebook.com>,
	"Jonathan Gray" <jgray@apache.org>
Date: Fri, 26 Nov 2010 23:13:39 -0000
Message-ID: <20101126231339.5602.12599@ip-10-202-7-187.ec2.internal>
Cc: dev@hbase.apache.org, "Ryan Rawson" <ryanobjc@gmail.com>,
	jiraposter@review.hbase.org, "Pranav Khaitan" <pranavkhaitan@gmail.com>
In-Reply-To: <20101126225445.19829.9138@ip-10-202-7-187.ec2.internal>
References: <20101126225445.19829.9138@ip-10-202-7-187.ec2.internal>


> On 2010-11-26 14:54:45, Ryan Rawson wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 1373
> > <http://review.cloudera.org/r/1252/diff/1/?file=3D17712#file17712line13=
73>
> >
> >     what are all the consequences for not sorting by type when using KV=
Comparator?  Does this mean we might create HFiles that not sorted properly=
, because the HFile comparator uses the KeyComparator directly with ignoreT=
ype =3D false. =

> >     =

> >     While in memstore we can rely on memstoreTS to roughly order by ins=
ertion time, and the Put/Delete should probably work in that situation, you=
 are talking about modifiying a pretty core and important concept in how we=
 sort things.
> >     =

> >     There are other ways to reconcile bugs like this, one of them is to=
 extend the memstoreTS concept into the HFile and use that to reconcile dur=
ing reads.  There is another JIRA where I proposed this.  =

> >     =

> >     If we are talking about 0.92 and beyond I'd prefer building a solid=
 base rather than dangerous hacks like this.  Our unit tests are not extrem=
ely extensive, so while they might pass, that doesnt guarantee lack of bad =
behaviour later on.
> >

Agree. As I mentioned, this is a major change and more thought needs to be =
given to it.

However, to resolve issues like HBASE-3276, we need either such a change or=
 extend the memstoreTS concept to HFile as you mentioned.

About consequences, I don't see anything negative here. This change only af=
fects the sorting of keys having same row, col, timestamp. After this chang=
e, all keys with the same row, col, ts will be sorted purely based on the o=
rder in which they were inserted. When a memstore is flushed to HFile, the =
memstoreTS takes care of ordering. During compactions, the KeyValueHeap bre=
aks ties by using the sequence ids of storefiles. =


- Pranav


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1252/#review1993
-----------------------------------------------------------


On 2010-11-26 07:47:02, Pranav Khaitan wrote:
> =

> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1252/
> -----------------------------------------------------------
> =

> (Updated 2010-11-26 07:47:02)
> =

> =

> Review request for hbase, Jonathan Gray and Kannan Muthukkaruppan.
> =

> =

> Summary
> -------
> =

> This is a design change suggested in HBASE-3276 so adequate thought shoul=
d be given before proceeding. =

> =

> The main code change is just one line which is to ignore key type while d=
oing KV comparisons. When the key type is ignored, then all the keys for th=
e same timestamp are sorted according the order in which they were interest=
ed. It is still ensured that the delete family and delete column will be at=
 the top because they have the default column name and default timestamp.
> =

> =

> This addresses bug HBASE-3276.
>     http://issues.apache.org/jira/browse/HBASE-3276
> =

> =

> Diffs
> -----
> =

>   trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java 1039233 =

>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/KeyValueScanFi=
xture.java 1039233 =

>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScann=
er.java 1039233 =

> =

> Diff: http://review.cloudera.org/r/1252/diff
> =

> =

> Testing
> -------
> =

> Test cases added. Since there is a change in semantics, some previous tes=
ts were failing because of this change. Those tests have been modified to t=
est the newer behavior.
> =

> =

> Thanks,
> =

> Pranav
> =

>