[
https://issues.apache.org/jira/browse/HBASE-4536?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13128077#comment-13128077
]
[email protected] commented on HBASE-4536:
------------------------------------------------------
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. > Great stuff.
Thanks for the thorough review. I since wrote a bunch of more tests and that
way found some more problems.
I'll add comments below and also attach a new patch soon.
What I do not like about this patch is that "entropy" in the ScanQueryMatcher
now. There are too many of statements and the logic is very hard to follow. On
the hand I have not found a way to make it simpler.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java,
line 322
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49403#file49403line322>
bq. >
bq. > So you made this change to make it so user doesn't have condition
where they are scratching their head trying to figure why something is not
being let go though TTL is four hours?
Exactly. MinVersion = MaxVersion makes no sense. I can do that in a separate
jira and for 0.92 too.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java,
line 556
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49403#file49403line556>
bq. >
bq. > Is this value read often? If so you might want to cache it.
Profiling these things can sometimes show up as costing.
bq. >
bq. > Not important but if you get to make new patch do
bq. >
bq. > if (test) block
bq. >
bq. > or
bq. >
bq. > if (test) {
bq. > block
bq. > }
bq. >
bq. > rather than the
bq. >
bq. > if (test)
bq. > block
bq. >
bq. > you have.
This is only read by the Store.<init>, so should be OK.
Absolutely agree of the if layout, this somehow sneaked in here.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Scan.java,
line 86
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49404#file49404line86>
bq. >
bq. > Let this be the convention from here on out. Maybe even two leading
underscores and no trailing underscore... but whatever, let this be the
convention for system configs in attribute maps going forward.
I was thinking to document this in the Javadoc of Attributes.java. There's
already DEFAULT_CLUSTER_ID in puts, which has an attribute prefixes by on _, I
can change that to two _. The trailing underscore does not matter, I guess.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Scan.java,
line 711
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49404#file49404line711>
bq. >
bq. > Good.
bq. >
bq. > There is a real old feature request in hbase that asked for this..
let me find the issue.... I can't find it.... user wanted to see deletes too in
a scan.
bq. >
bq. > Then with Benoit a week or so back we had to look in hfile because
we didn't trust a delete.... we thought there stuff behind it. We could have
used this facility to show all that was in hbase (if we'd enabled this new
feature of course).
bq. >
bq. > I think this facility should be on by default. Whats the downsides?
We don't let go of stuff if < maxversions?
bq. >
bq. > How you going to do versions now? If I ask for a scan beyond a
delete will I see maxversions only because thats all we keep between deletes?
Will it be the case that I will end up having:
bq. >
bq. > maxversions... delete marker .... maxversions ... delete marker ....
maxversions...
bq. >
bq. > and so on if this facility is enabled?
bq. >
bq. > I won't have:
bq. >
bq. > as many versions as was written but we only return
maxversions......delete marker...... as many versions as were written......
delete marker......
bq. >
bq. > Is that right? (If you follow me).
bq. >
bq. > Say 'garbage collected' in the comment rather than just 'collected'
They way it works now, is that it is guaranteed to see all *relevant* deletes.
Say VERSIONS=2 and you have
put, put, put, delete, delete (all of the same row). You would only get the
puts back, because the last put and the deletes are guaranteed to have no
effect.
Maybe also have something like: delete, delete, delete, put, delete. In that
case you get everything, as it cannot be shown that any of the deletes is not
significant.
In any case, deletes do not count towards the version number of following puts
or deletes.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java,
line 59
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49407#file49407line59>
bq. >
bq. > So when I am doing a check for max versions = 3 and I have for a
single cell:
bq. >
bq. >
bq. > put ... put....delete....put
bq. >
bq. >
bq. > If I keep delete markers and do a raw scan, I'll get: p p d
bq. >
bq. > If I have keep delete markers but not a raw scan I get: p p
bq. >
bq. > If I do not have keep delete markers and its not an ordinary scan: p
p
bq. >
bq. > Something like that?
Exactly. That's why the login here is a bit convoluted now.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java,
line 60
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49407#file49407line60>
bq. >
bq. > I'm not sure I get what this one is about.
This is what enabled "ordinary" scan to see rows behind a delete marker. Ted
suggested to relabel those to "time range" queries.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java,
line 222
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49407#file49407line222>
bq. >
bq. > I don't understand whats up here. Why we changing timerange if
seePastDeleteMarker is set?
Ah yes. This is why the scanner needs to know whether we want to be able scan
past delete markers. The *afterTimeRange will add all delete markers > some TS
to the set of deletes and hence hide all earlier puts. In order to allow range
queries before a delete marker the marker must only be included if it is inside
the time range of the query.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java,
line 232
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49407#file49407line232>
bq. >
bq. > Is 'keepDeletedRows' a bad name for this variable? Should it be
keepDeletedCells?
yes :)
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java,
line 235
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49407#file49407line235>
bq. >
bq. > So, this is a delete kv at this stage of the processing? So, if its
older than any put in any storefile on major compactions, we can let it go
since has no effect? Thats good.
For this to work I added a new meta data item to StoreFiles: the timestamp of
the earliest put found. Not pretty, but I cannot think of anything else.
This is used for family delete marker because they are always *before all* puts
regardless of timestamp :(
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java,
line 243
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49407#file49407line243>
bq. >
bq. > This makes me nervous. All old tests pass on this code? You have a
unit test to test that we are doing the right thing in here both for the old
and the new handling?
All tests pass :)
This is because delete markers can now fall through and be subject to this test
as well.
Looking at that now, though, this not necessary. I'll remove it, and rerun my
test.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java,
line 73
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49408#file49408line73>
bq. >
bq. > Again, bad name for param.
Changed already.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java,
line 77
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49408#file49408line77>
bq. >
bq. > Is this right? We should count it as a version if keepdeletes is
on? Unless this flag is saying 'keepdeletes' and not 'this is a delete kv' as
I'm interpreting it).
That would certainly make things easier. But would it be right?
If VERSIONS is 3 and I have
delete, put, delete, put, then the last put would be forever unreachable.
If we think that's OK, I can simplify the code.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java,
line 487
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49409#file49409line487>
bq. >
bq. > No biggie... previous formatting was better.
Heh, OK.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java,
line 371
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49411#file49411line371>
bq. >
bq. > Why we move this to the end?
Oops... Leftover from other attempts. Will move it back.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeepDeletes.java,
line 133
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49415#file49415line133>
bq. >
bq. > Good test
I add way more tests... new patch coming soon.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java,
line 127
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49410#file49410line127>
bq. >
bq. > This belongs in HCD rather than up here in this global HConstants?
Should it? It's very store file specific.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ColumnTracker.java,
line 50
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49405#file49405line50>
bq. >
bq. > Bad name for a param. This is name you'd give the getter for a
param named 'delete'.
bq. >
bq. > What is this param doing? Is this the raw attribute or is it from
HCD saying keep deletes? Make it clear in javadoc.
I changed the logic now to actually pass in the type of the kv.
bq. On 2011-10-15 04:00:08, Michael Stack wrote:
bq. >
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java,
line 106
bq. > <https://reviews.apache.org/r/2178/diff/7/?file=49407#file49407line106>
bq. >
bq. > These params on an SQM are starting to build up.
Yes... And I do not like it... At all!
Maybe we should pass in the Store itself.
I was also thinking about an enum for the scan type too. There are seven
different cases:
1. major colection, not keeping deleted rows
2. major colection, keeping deleted rows
3. minor colection, not keeping deleted rows
4. minor colection, keeping deleted rows
5. ordinary scan, with delete peeking
6. ordinary scan, without delete peeking
7. raw scan
- Lars
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2178/#review2609
-----------------------------------------------------------
On 2011-10-12 04:55:53, Lars Hofhansl wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2178/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-12 04:55:53)
bq.
bq.
bq. Review request for hbase, Ted Yu and Jonathan Gray.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. HBase timerange Gets and Scans allow to do "timetravel" in HBase. I.e.
look at the state of the data at any point in the past, provided the data is
still around.
bq. This did not work for deletes, however. Deletes would always mask all puts
in the past.
bq. This change adds a flag that can be on HColumnDescriptor to enable
retention of deleted rows.
bq. These rows are still subject to TTL and/or VERSIONS.
bq.
bq. This changes the following:
bq. 1. There is a new flag on HColumnDescriptor enabling that behavior.
bq. 2. Allow gets/scans with a timerange to retrieve rows hidden by a delete
marker, if the timerange does not include the delete marker.
bq. 3. Do not unconditionally collect all deleted rows during a compaction.
bq. 4. Allow a "raw" Scan, which retrieves all delete markers and deleted rows.
bq.
bq. The change is small'ish, but the logic is intricate, so please review
carefully.
bq.
bq.
bq. This addresses bug HBASE-4536.
bq. https://issues.apache.org/jira/browse/HBASE-4536
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Scan.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ColumnTracker.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
1181616
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/hbase/admin.rb
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeepDeletes.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMinVersions.java
1181616
bq.
http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWildcardColumnTracker.java
1181616
bq.
bq. Diff: https://reviews.apache.org/r/2178/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All tests pass now.
bq.
bq.
bq. Thanks,
bq.
bq. Lars
bq.
bq.
> Allow CF to retain deleted rows
> -------------------------------
>
> Key: HBASE-4536
> URL: https://issues.apache.org/jira/browse/HBASE-4536
> Project: HBase
> Issue Type: New Feature
> Components: regionserver
> Affects Versions: 0.92.0
> Reporter: Lars Hofhansl
> Assignee: Lars Hofhansl
> Fix For: 0.94.0
>
>
> Parent allows for a cluster to retain rows for a TTL or keep a minimum number
> of versions.
> However, if a client deletes a row all version older than the delete tomb
> stone will be remove at the next major compaction (and even at memstore flush
> - see HBASE-4241).
> There should be a way to retain those version to guard against software error.
> I see two options here:
> 1. Add a new flag HColumnDescriptor. Something like "RETAIN_DELETED".
> 2. Folds this into the parent change. I.e. keep minimum-number-of-versions of
> versions even past the delete marker.
> #1 would allow for more flexibility. #2 comes somewhat naturally with parent
> (from a user viewpoint)
> Comments? Any other options?
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
