You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Daniel Ploeg <dp...@gmail.com> on 2010/05/25 12:07:26 UTC
Review Request: HBASE-2578
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/
-----------------------------------------------------------
Review request for hbase.
Summary
-------
HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
Diffs
-----
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
Diff: http://review.hbase.org/r/83/diff
Testing
-------
Thanks,
Daniel
Re: Review Request: HBASE-2578
Posted by Ryan Rawson <ry...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/#review62
-----------------------------------------------------------
What other possible approaches could we take to solving this issue? I seem to recalling a different strategy but I cannot recall it right now. We are really not set up for DI right now, and the ultimate direction of this patch is a fully dependency injected solution.
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
<http://review.hbase.org/r/83/#comment345>
this seems overly HRegion specific - what is the "standard" mechanism for providing mocked out systems level time-based replacements?
src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java
<http://review.hbase.org/r/83/#comment344>
lets not use assertEquals(String,int,int) and just use assertEquals(int,int).
- Ryan
On 2010-05-25 12:40:01, Daniel Ploeg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/83/
> -----------------------------------------------------------
>
> (Updated 2010-05-25 12:40:01)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
> The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
> One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
>
>
> This addresses bug HBASE-2578.
> http://issues.apache.org/jira/browse/HBASE-2578
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
> src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
> src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
>
> Diff: http://review.hbase.org/r/83/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel
>
>
Re: Review Request: HBASE-2578
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/#review65
-----------------------------------------------------------
Ship it!
Implementation is well done. The need for this though strikes me as odd. So I'm +1 on the patch but let me go comment up on the bigger issue I see up in the JIRA.
- stack
On 2010-05-25 12:40:01, Daniel Ploeg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/83/
> -----------------------------------------------------------
>
> (Updated 2010-05-25 12:40:01)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
> The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
> One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
>
>
> This addresses bug HBASE-2578.
> http://issues.apache.org/jira/browse/HBASE-2578
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
> src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
> src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
>
> Diff: http://review.hbase.org/r/83/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel
>
>
Re: Review Request: HBASE-2578
Posted by Paul Cowan <co...@aconex.com>.
On 26/05/2010 4:26 PM, Ryan Rawson wrote:
> Sounds like we want #1: replaceable singleton.
>
> does that sound right?
Yep. :)
Hence volatile should be all you need in this case. It gets complicated
if there's additional thread-safe logic needed in the getter or setter,
but in this case that doesn't apply and volatile should give you all you
need.
Cheers,
Paul
Re: Review Request: HBASE-2578
Posted by Ryan Rawson <ry...@gmail.com>.
Sounds like we want #1: replaceable singleton.
does that sound right?
On Tue, May 25, 2010 at 9:16 PM, Paul Cowan <co...@aconex.com> wrote:
> On 26/05/2010 1:54 PM, Ryan Rawson wrote:
>>
>> Thanks for that salient comment. Perhaps someone can give us the right
>> pattern for no lock Singleton initialization.
>
> There are really only two things which complicate thread-safe singletons:
>
> 1) Having a setter, so you can replace the singleton
> 2) Lazy initialization. The only excuse for this is having a
> horribly-expensive-to-create singleton, which may not even be used, and you
> don't under any circumstances wish to create it unless it's needed. This is
> hard to get right, and 99% of the time you should just instantiate it at
> class initializion time and forget it.
>
> In this case, 1) applies, but 2) doesn't; luckily 1) is a lot easier to get
> right than 2).
>
> Simplest 'correct' patterns are:
>
>
> Basic Singleton (neither #1 nor #2 apply)
> -------------------------------------------
>
> public class Thing {
> private static final Thing INSTANCE = new AwesomeThing();
>
> public Thing get() {
> return INSTANCE;
> }
> }
>
> this is easy, simple, performant (no synchronization or locking at all) and
> 100% thread-safe (as long as INSTANCE is final).
>
>
>
> Replacable Singleton (#1 applies)
> -------------------------------------------
> public class Thing {
> private static volatile Thing instance = new AwesomeThing(); // Note:
> volatile
>
> public Thing get() {
> return instance;
> }
>
> public void set(Thing newThing) {
> this.instance = newThing;
> }
> }
>
> Nearly as easy, all that's required here is a volatile instance which gives
> you the correct happens-before relationship.
>
>
>
> Lazy-Initialized Singleton (#2 applies)
> -------------------------------------------
> Avoid if possible, but if not, one of:
>
> Synchronized Getter:
> public synchronized Thing get() {
> if (instance == null) {
> instance = new AwesomeThing();
> }
> return instance;
> }
>
> Double-checked locking (ONLY works in Java 5+)
> public Thing get() {
> if (instance == null) {
> synchronized (ThingHolder.class) {
> if (instance == null) { // Yes, you need this
> instance = new AwesomeThing();
> }
> }
> }
> return instance;
>
> Use java classloading locking guarantees + holder class (see: Effective
> Java)
> class ThingHolder {
> private static final Thing INSTANCE = new AwesomeThing();
> }
>
> class Thing {
> public Thing getThing() {
> return ThingHolder.instance;
> }
> }
>
>
>
> Lazy-Initialized Settable Singleton (#1 + #2 apply)
> -------------------------------------------
> You almost certainly don't need this. You think you do, but you don't. Find
> a better way. :)
>
>
> Hope that helps.
>
> Cheers,
>
> Paul
>
Re: Review Request: HBASE-2578
Posted by Paul Cowan <co...@aconex.com>.
On 26/05/2010 1:54 PM, Ryan Rawson wrote:
> Thanks for that salient comment. Perhaps someone can give us the right
> pattern for no lock Singleton initialization.
There are really only two things which complicate thread-safe singletons:
1) Having a setter, so you can replace the singleton
2) Lazy initialization. The only excuse for this is having a
horribly-expensive-to-create singleton, which may not even be used, and
you don't under any circumstances wish to create it unless it's needed.
This is hard to get right, and 99% of the time you should just
instantiate it at class initializion time and forget it.
In this case, 1) applies, but 2) doesn't; luckily 1) is a lot easier to
get right than 2).
Simplest 'correct' patterns are:
Basic Singleton (neither #1 nor #2 apply)
-------------------------------------------
public class Thing {
private static final Thing INSTANCE = new AwesomeThing();
public Thing get() {
return INSTANCE;
}
}
this is easy, simple, performant (no synchronization or locking at all)
and 100% thread-safe (as long as INSTANCE is final).
Replacable Singleton (#1 applies)
-------------------------------------------
public class Thing {
private static volatile Thing instance = new AwesomeThing(); //
Note: volatile
public Thing get() {
return instance;
}
public void set(Thing newThing) {
this.instance = newThing;
}
}
Nearly as easy, all that's required here is a volatile instance which
gives you the correct happens-before relationship.
Lazy-Initialized Singleton (#2 applies)
-------------------------------------------
Avoid if possible, but if not, one of:
Synchronized Getter:
public synchronized Thing get() {
if (instance == null) {
instance = new AwesomeThing();
}
return instance;
}
Double-checked locking (ONLY works in Java 5+)
public Thing get() {
if (instance == null) {
synchronized (ThingHolder.class) {
if (instance == null) { // Yes, you need this
instance = new AwesomeThing();
}
}
}
return instance;
Use java classloading locking guarantees + holder class (see: Effective
Java)
class ThingHolder {
private static final Thing INSTANCE = new AwesomeThing();
}
class Thing {
public Thing getThing() {
return ThingHolder.instance;
}
}
Lazy-Initialized Settable Singleton (#1 + #2 apply)
-------------------------------------------
You almost certainly don't need this. You think you do, but you don't.
Find a better way. :)
Hope that helps.
Cheers,
Paul
Re: Review Request: HBASE-2578
Posted by Todd Lipcon <to...@cloudera.com>.
Why not just initialize it from a static {} block, then in a test if we want
to change it, we change it?
On Tue, May 25, 2010 at 8:54 PM, Ryan Rawson <ry...@gmail.com> wrote:
> Thanks for that salient comment. Perhaps someone can give us the right
> pattern for no lock Singleton initialization.
>
> On May 25, 2010 8:49 PM, "Paul Cowan" <co...@aconex.com> wrote:
> > On 26/05/2010 1:15 PM, Daniel Ploeg wrote:
> >>>> this is a lot of synchronization for what ends up being on the fast
> path of every query, perhaps there is a non synchronized manner in which we
> can accomplish this? the use of volatile might help here
> >>
> >> thanks Ryan. I think I might use AtomicReference instead - can simplify
> even further this way. I'll upload another patch a bit later.
> >
> > Sorry, no real stake in this review but just a curious third-party: is
> > there any need for even the AtomicReference?
> >
> > Making 'delegate' volatile would completely remove the need for the
> > synchronization in getDelegate(), and the lock on injectEdge(). The only
> > exception is that without synchronization, the null-check-and-default in
> > getDelegate() would be subject to race conditions. Given the desire to
> > reduce synchronization, and the fact that delegate is assigned to
> > default at construction time, I'd humbly suggest that the "if (delegate
> > == null)" be removed from getDelegate(), and instead change injectEdge()
> > to reject null values (or call reset() if you want to keep the effective
> > existing semantics that injectEdge(null) returns to a
> > DefaultEnvironmentEdge).
> >
> > Then the ReentrantLock can be removed, no synchronization is required,
> > and one volatile variable is all that's needed.
> >
> > (Incidentally, I'm not sure what the ReentrantLock is actually FOR --
> > reference assignment is atomic in Java, so it's not protecting against
> > any race conditions; all it could be useful for is a happens-before
> > relationship with the get, but the two are using different locks -- the
> > synchronized get does not have a happens-before relationship with the
> > unlock. Either both should be synchronized, or both use the lock -- as
> > it is, the lock buys you nothing. Mind you with a simple volatile then
> > the happens-before relationship is correct anyway).
> >
> > Cheers,
> >
> > Paul
>
--
Todd Lipcon
Software Engineer, Cloudera
Re: Review Request: HBASE-2578
Posted by Ryan Rawson <ry...@gmail.com>.
Thanks for that salient comment. Perhaps someone can give us the right
pattern for no lock Singleton initialization.
On May 25, 2010 8:49 PM, "Paul Cowan" <co...@aconex.com> wrote:
> On 26/05/2010 1:15 PM, Daniel Ploeg wrote:
>>>> this is a lot of synchronization for what ends up being on the fast
path of every query, perhaps there is a non synchronized manner in which we
can accomplish this? the use of volatile might help here
>>
>> thanks Ryan. I think I might use AtomicReference instead - can simplify
even further this way. I'll upload another patch a bit later.
>
> Sorry, no real stake in this review but just a curious third-party: is
> there any need for even the AtomicReference?
>
> Making 'delegate' volatile would completely remove the need for the
> synchronization in getDelegate(), and the lock on injectEdge(). The only
> exception is that without synchronization, the null-check-and-default in
> getDelegate() would be subject to race conditions. Given the desire to
> reduce synchronization, and the fact that delegate is assigned to
> default at construction time, I'd humbly suggest that the "if (delegate
> == null)" be removed from getDelegate(), and instead change injectEdge()
> to reject null values (or call reset() if you want to keep the effective
> existing semantics that injectEdge(null) returns to a
> DefaultEnvironmentEdge).
>
> Then the ReentrantLock can be removed, no synchronization is required,
> and one volatile variable is all that's needed.
>
> (Incidentally, I'm not sure what the ReentrantLock is actually FOR --
> reference assignment is atomic in Java, so it's not protecting against
> any race conditions; all it could be useful for is a happens-before
> relationship with the get, but the two are using different locks -- the
> synchronized get does not have a happens-before relationship with the
> unlock. Either both should be synchronized, or both use the lock -- as
> it is, the lock buys you nothing. Mind you with a simple volatile then
> the happens-before relationship is correct anyway).
>
> Cheers,
>
> Paul
Re: Review Request: HBASE-2578
Posted by Paul Cowan <co...@aconex.com>.
On 26/05/2010 1:15 PM, Daniel Ploeg wrote:
>>> this is a lot of synchronization for what ends up being on the fast path of every query, perhaps there is a non synchronized manner in which we can accomplish this? the use of volatile might help here
>
> thanks Ryan. I think I might use AtomicReference instead - can simplify even further this way. I'll upload another patch a bit later.
Sorry, no real stake in this review but just a curious third-party: is
there any need for even the AtomicReference?
Making 'delegate' volatile would completely remove the need for the
synchronization in getDelegate(), and the lock on injectEdge(). The only
exception is that without synchronization, the null-check-and-default in
getDelegate() would be subject to race conditions. Given the desire to
reduce synchronization, and the fact that delegate is assigned to
default at construction time, I'd humbly suggest that the "if (delegate
== null)" be removed from getDelegate(), and instead change injectEdge()
to reject null values (or call reset() if you want to keep the effective
existing semantics that injectEdge(null) returns to a
DefaultEnvironmentEdge).
Then the ReentrantLock can be removed, no synchronization is required,
and one volatile variable is all that's needed.
(Incidentally, I'm not sure what the ReentrantLock is actually FOR --
reference assignment is atomic in Java, so it's not protecting against
any race conditions; all it could be useful for is a happens-before
relationship with the get, but the two are using different locks -- the
synchronized get does not have a happens-before relationship with the
unlock. Either both should be synchronized, or both use the lock -- as
it is, the lock buys you nothing. Mind you with a simple volatile then
the happens-before relationship is correct anyway).
Cheers,
Paul
Re: Review Request: HBASE-2578
Posted by Daniel Ploeg <dp...@gmail.com>.
> On 2010-05-25 18:15:35, Ryan Rawson wrote:
> > src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java, line 41
> > <http://review.hbase.org/r/83/diff/2/?file=643#file643line41>
> >
> > this is a lot of synchronization for what ends up being on the fast path of every query, perhaps there is a non synchronized manner in which we can accomplish this? the use of volatile might help here
thanks Ryan. I think I might use AtomicReference instead - can simplify even further this way. I'll upload another patch a bit later.
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/#review72
-----------------------------------------------------------
On 2010-05-25 17:49:37, Daniel Ploeg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/83/
> -----------------------------------------------------------
>
> (Updated 2010-05-25 17:49:37)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
> The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
> One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
>
>
> This addresses bug HBASE-2578.
> http://issues.apache.org/jira/browse/HBASE-2578
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
> src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
> src/test/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManagerTestHelper.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestEnvironmentEdgeManager.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
>
> Diff: http://review.hbase.org/r/83/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel
>
>
Re: Review Request: HBASE-2578
Posted by Ryan Rawson <ry...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/#review72
-----------------------------------------------------------
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java
<http://review.hbase.org/r/83/#comment393>
this is a lot of synchronization for what ends up being on the fast path of every query, perhaps there is a non synchronized manner in which we can accomplish this? the use of volatile might help here
- Ryan
On 2010-05-25 17:49:37, Daniel Ploeg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/83/
> -----------------------------------------------------------
>
> (Updated 2010-05-25 17:49:37)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
> The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
> One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
>
>
> This addresses bug HBASE-2578.
> http://issues.apache.org/jira/browse/HBASE-2578
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
> src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
> src/test/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManagerTestHelper.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestEnvironmentEdgeManager.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
>
> Diff: http://review.hbase.org/r/83/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel
>
>
Re: Review Request: HBASE-2578
Posted by Ryan Rawson <ry...@gmail.com>.
Would you mind using mockito? I am trying to move us there and this
would be a good first step.
Thanks!
On Thu, May 27, 2010 at 5:29 PM, Daniel Ploeg <dp...@gmail.com> wrote:
>
>
>> On 2010-05-27 16:45:23, Ryan Rawson wrote:
>> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2701
>> > <http://review.hbase.org/r/83/diff/3/?file=690#file690line2701>
>> >
>> > I'm thinking perhaps a Static interface could be used instead of the getDelegate().currentTimeMillis() that way the individual call sites could say:
>> >
>> > EnvironmentEdge.currentTimeMillis()
>> > instead of
>> > System.currentTimeMillis()
>> >
>> > but the static calls would be a simple wrapper around the existing getDelegate mechanisms... this would just make the code more explicit and easier to follow for future generations.
>> >
>> > OTher than that, looking great! Thanks for making all the changes!
>
> Ok sounds good. The EnvironmentEdge is the interface, so I think I'll need to put the static method on the manager. The call would be:
>
> EnvironmentEdgeManager.currentTimeMillis()
>
> I'm adding a mocking library to the pom (EasyMock) to help with the testing.
>
>
> - Daniel
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/83/#review88
> -----------------------------------------------------------
>
>
> On 2010-05-27 14:51:48, Daniel Ploeg wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> http://review.hbase.org/r/83/
>> -----------------------------------------------------------
>>
>> (Updated 2010-05-27 14:51:48)
>>
>>
>> Review request for hbase.
>>
>>
>> Summary
>> -------
>>
>> HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
>> The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
>> One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
>>
>>
>> This addresses bug HBASE-2578.
>> http://issues.apache.org/jira/browse/HBASE-2578
>>
>>
>> Diffs
>> -----
>>
>> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
>> src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
>> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
>> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java PRE-CREATION
>> src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
>> src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
>> src/test/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManagerTestHelper.java PRE-CREATION
>> src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
>> src/test/java/org/apache/hadoop/hbase/util/TestEnvironmentEdgeManager.java PRE-CREATION
>> src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
>>
>> Diff: http://review.hbase.org/r/83/diff
>>
>>
>> Testing
>> -------
>>
>>
>> Thanks,
>>
>> Daniel
>>
>>
>
>
Re: Review Request: HBASE-2578
Posted by Daniel Ploeg <dp...@gmail.com>.
> On 2010-05-27 16:45:23, Ryan Rawson wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2701
> > <http://review.hbase.org/r/83/diff/3/?file=690#file690line2701>
> >
> > I'm thinking perhaps a Static interface could be used instead of the getDelegate().currentTimeMillis() that way the individual call sites could say:
> >
> > EnvironmentEdge.currentTimeMillis()
> > instead of
> > System.currentTimeMillis()
> >
> > but the static calls would be a simple wrapper around the existing getDelegate mechanisms... this would just make the code more explicit and easier to follow for future generations.
> >
> > OTher than that, looking great! Thanks for making all the changes!
Ok sounds good. The EnvironmentEdge is the interface, so I think I'll need to put the static method on the manager. The call would be:
EnvironmentEdgeManager.currentTimeMillis()
I'm adding a mocking library to the pom (EasyMock) to help with the testing.
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/#review88
-----------------------------------------------------------
On 2010-05-27 14:51:48, Daniel Ploeg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/83/
> -----------------------------------------------------------
>
> (Updated 2010-05-27 14:51:48)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
> The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
> One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
>
>
> This addresses bug HBASE-2578.
> http://issues.apache.org/jira/browse/HBASE-2578
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
> src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
> src/test/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManagerTestHelper.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestEnvironmentEdgeManager.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
>
> Diff: http://review.hbase.org/r/83/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel
>
>
Re: Review Request: HBASE-2578
Posted by Ryan Rawson <ry...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/#review88
-----------------------------------------------------------
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
<http://review.hbase.org/r/83/#comment510>
I'm thinking perhaps a Static interface could be used instead of the getDelegate().currentTimeMillis() that way the individual call sites could say:
EnvironmentEdge.currentTimeMillis()
instead of
System.currentTimeMillis()
but the static calls would be a simple wrapper around the existing getDelegate mechanisms... this would just make the code more explicit and easier to follow for future generations.
OTher than that, looking great! Thanks for making all the changes!
- Ryan
On 2010-05-27 14:51:48, Daniel Ploeg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/83/
> -----------------------------------------------------------
>
> (Updated 2010-05-27 14:51:48)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
> The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
> One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
>
>
> This addresses bug HBASE-2578.
> http://issues.apache.org/jira/browse/HBASE-2578
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
> src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
> src/test/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManagerTestHelper.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestEnvironmentEdgeManager.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
>
> Diff: http://review.hbase.org/r/83/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel
>
>
Re: Review Request: HBASE-2578
Posted by Daniel Ploeg <dp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/
-----------------------------------------------------------
(Updated 2010-05-31 14:18:59.665128)
Review request for hbase.
Changes
-------
Changed easymock to mockito :)
Summary
-------
HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
This addresses bug HBASE-2578.
http://issues.apache.org/jira/browse/HBASE-2578
Diffs (updated)
-----
pom.xml 0a009cf
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
src/test/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManagerTestHelper.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestEnvironmentEdgeManager.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
Diff: http://review.hbase.org/r/83/diff
Testing
-------
Thanks,
Daniel
Re: Review Request: HBASE-2578
Posted by Daniel Ploeg <dp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/
-----------------------------------------------------------
(Updated 2010-05-27 19:16:50.127955)
Review request for hbase.
Changes
-------
Added static interface
Summary
-------
HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
This addresses bug HBASE-2578.
http://issues.apache.org/jira/browse/HBASE-2578
Diffs (updated)
-----
pom.xml 0a009cf
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
src/test/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManagerTestHelper.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestEnvironmentEdgeManager.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
Diff: http://review.hbase.org/r/83/diff
Testing
-------
Thanks,
Daniel
Re: Review Request: HBASE-2578
Posted by Daniel Ploeg <dp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/
-----------------------------------------------------------
(Updated 2010-05-27 14:51:48.073426)
Review request for hbase.
Changes
-------
Using volatile and resetting with default on null injection.Thanks to Paul and Ryan for feedback on the mailing list.
Summary
-------
HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
This addresses bug HBASE-2578.
http://issues.apache.org/jira/browse/HBASE-2578
Diffs (updated)
-----
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
src/test/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManagerTestHelper.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestEnvironmentEdgeManager.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
Diff: http://review.hbase.org/r/83/diff
Testing
-------
Thanks,
Daniel
Re: Review Request: HBASE-2578
Posted by Daniel Ploeg <dp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/
-----------------------------------------------------------
(Updated 2010-05-25 17:49:37.154103)
Review request for hbase.
Changes
-------
Based on the feedback, I have updated the patch. This solution is more generic and can be used anywhere in the application.
Summary
-------
HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
This addresses bug HBASE-2578.
http://issues.apache.org/jira/browse/HBASE-2578
Diffs (updated)
-----
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
src/test/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManagerTestHelper.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestEnvironmentEdgeManager.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
Diff: http://review.hbase.org/r/83/diff
Testing
-------
Thanks,
Daniel
Re: Review Request: HBASE-2578
Posted by Todd Lipcon <to...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/
-----------------------------------------------------------
(Updated 2010-05-25 12:40:01.904248)
Review request for hbase.
Changes
-------
Adding bug field so this shows up on JIRA
Summary
-------
HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
This addresses bug HBASE-2578.
http://issues.apache.org/jira/browse/HBASE-2578
Diffs
-----
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
Diff: http://review.hbase.org/r/83/diff
Testing
-------
Thanks,
Daniel
Re: Review Request: HBASE-2578
Posted by Daniel Ploeg <dp...@gmail.com>.
> On 2010-05-25 12:39:39, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java, line 17
> > <http://review.hbase.org/r/83/diff/1/?file=587#file587line17>
> >
> > may need to be synchronized, or use AtomicLong
Thanks Todd.
- I'm modifying the HowToContribute page and didn't see a reference to the inclusion of the license header there or on the Code review page. I haven't committed my next change to the HowToContribute page yet, so I'm adding a reference there (plus modifying my files).
- The singleton approach I think might be better. I thought that having the instance variable smelled a bit, that's why I asked about the fixed overhead. I'll make some changes.
- IncrementingEE - I'll make some sync type changes here.
I'll upload a new patch later today with these changes.
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/#review56
-----------------------------------------------------------
On 2010-05-25 12:40:01, Daniel Ploeg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/83/
> -----------------------------------------------------------
>
> (Updated 2010-05-25 12:40:01)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
> The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
> One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
>
>
> This addresses bug HBASE-2578.
> http://issues.apache.org/jira/browse/HBASE-2578
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
> src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
> src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
>
> Diff: http://review.hbase.org/r/83/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel
>
>
Re: Review Request: HBASE-2578
Posted by Todd Lipcon <to...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/83/#review56
-----------------------------------------------------------
Looks good. Few notes:
- Please add the Apache license header to all the new files
- Do we really need to set this up as an instance variable for each HRegion? It seems like it may be more useful as a global singleton - I'm wondering whether we're going to run into issues if we try to extend this to do tests that cross HRegion->Store->StoreFile boundaries. What do you think?
src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java
<http://review.hbase.org/r/83/#comment314>
may need to be synchronized, or use AtomicLong
- Todd
On 2010-05-25 03:07:25, Daniel Ploeg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/83/
> -----------------------------------------------------------
>
> (Updated 2010-05-25 03:07:25)
>
>
> Review request for hbase.
>
>
> Summary
> -------
>
> HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
> The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
> One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
> src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java PRE-CREATION
> src/main/java/org/apache/hadoop/hbase/util/IncrementingEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java d3716d6
> src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java PRE-CREATION
> src/test/java/org/apache/hadoop/hbase/util/TestIncrementingEnvironmentEdge.java PRE-CREATION
>
> Diff: http://review.hbase.org/r/83/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel
>
>