Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Michael checked the checkAndPut which doesn't expose timestamp. A variant of checkAndPut should be created to expose timestamp which is written into a column specified by additional parameters.

        Activity

        Hide
        Jonathan Gray added a comment -

        Is this jira for modifying CheckAndPut or using it with META? The name doesn't match the description. And I'm not sure this really a sub-task as much as it's a related task.

        Show
        Jonathan Gray added a comment - Is this jira for modifying CheckAndPut or using it with META? The name doesn't match the description. And I'm not sure this really a sub-task as much as it's a related task.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/
        -----------------------------------------------------------

        (Updated 2011-09-29 21:10:29.425308)

        Review request for hbase.

        Changes
        -------

        Fixed Summary.

        Summary (updated)
        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.
        https://issues.apache.org/jira/browse/hbase-4507

        Diffs


        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 7cbdb98
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0c06f4f
        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing
        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 21:10:29.425308) Review request for hbase. Changes ------- Fixed Summary. Summary (updated) ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 7cbdb98 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0c06f4f src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        stack added a comment -

        I posted first cut at a patch that adds the timestamp. Also has guards against an update going in with same timestamp as the update its overwriting, the case Ming outlines at tail of hbase-4497.

        Show
        stack added a comment - I posted first cut at a patch that adds the timestamp. Also has guards against an update going in with same timestamp as the update its overwriting, the case Ming outlines at tail of hbase-4497.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2176
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
        <https://reviews.apache.org/r/2118/#comment5070>

        expected is not needed here.

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
        <https://reviews.apache.org/r/2118/#comment5071>

        Should be existence.

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
        <https://reviews.apache.org/r/2118/#comment5072>

        Should be executed.

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2118/#comment5073>

        I don't see this parameter below.
        I think these four lines can be moved into the body of the method.

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2118/#comment5076>

        Writable parameter is missing here.
        It would be nice to add it now.

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2118/#comment5078>

        Should be checkAndPut

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2118/#comment5079>

        How about naming this method doTimestampsClash ?

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2118/#comment5081>

        I wish there is a better way to handle this situation

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2118/#comment5080>

        'that of the' can be omitted.

        • Ted

        On 2011-09-29 21:10:29, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 21:10:29)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 7cbdb98

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0c06f4f

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2176 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java < https://reviews.apache.org/r/2118/#comment5070 > expected is not needed here. src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java < https://reviews.apache.org/r/2118/#comment5071 > Should be existence. src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java < https://reviews.apache.org/r/2118/#comment5072 > Should be executed. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2118/#comment5073 > I don't see this parameter below. I think these four lines can be moved into the body of the method. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2118/#comment5076 > Writable parameter is missing here. It would be nice to add it now. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2118/#comment5078 > Should be checkAndPut src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2118/#comment5079 > How about naming this method doTimestampsClash ? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2118/#comment5081 > I wish there is a better way to handle this situation src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2118/#comment5080 > 'that of the' can be omitted. Ted On 2011-09-29 21:10:29, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 21:10:29) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 7cbdb98 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0c06f4f src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2185
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2118/#comment5089>

        Since expectedTimestampOfLatestVersion would only be used for use case from HBASE04497, I think we just need a reliable way of enforcing this 1 ms advance.
        Maybe spinlocking till EnvironmentEdgeManager.currentTimeMillis() returns now+1 ?

        • Ted

        On 2011-09-29 21:10:29, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 21:10:29)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 7cbdb98

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0c06f4f

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2185 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2118/#comment5089 > Since expectedTimestampOfLatestVersion would only be used for use case from HBASE04497, I think we just need a reliable way of enforcing this 1 ms advance. Maybe spinlocking till EnvironmentEdgeManager.currentTimeMillis() returns now+1 ? Ted On 2011-09-29 21:10:29, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 21:10:29) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 7cbdb98 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0c06f4f src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-29 21:54:42, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1942

        > <https://reviews.apache.org/r/2118/diff/1/?file=46471#file46471line1942>

        >

        > How about naming this method doTimestampsClash ?

        Because I wanted to follow beans convention for naming methods that have boolean return (its not the best english but tells you more about what to expect of the method than a 'do').

        On 2011-09-29 21:54:42, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1948

        > <https://reviews.apache.org/r/2118/diff/1/?file=46471#file46471line1948>

        >

        > I wish there is a better way to handle this situation

        This situation is going to be extremely rare. If it happens at all, we do this +1 on the ts. I prefer this to a spin-wait holding up the update.

        • Michael

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2176
        -----------------------------------------------------------

        On 2011-09-29 21:10:29, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 21:10:29)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 7cbdb98

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0c06f4f

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-29 21:54:42, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1942 > < https://reviews.apache.org/r/2118/diff/1/?file=46471#file46471line1942 > > > How about naming this method doTimestampsClash ? Because I wanted to follow beans convention for naming methods that have boolean return (its not the best english but tells you more about what to expect of the method than a 'do'). On 2011-09-29 21:54:42, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1948 > < https://reviews.apache.org/r/2118/diff/1/?file=46471#file46471line1948 > > > I wish there is a better way to handle this situation This situation is going to be extremely rare. If it happens at all, we do this +1 on the ts. I prefer this to a spin-wait holding up the update. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2176 ----------------------------------------------------------- On 2011-09-29 21:10:29, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 21:10:29) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 7cbdb98 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0c06f4f src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/
        -----------------------------------------------------------

        (Updated 2011-09-29 23:52:43.167788)

        Review request for hbase.

        Changes
        -------

        Address Ted's feedback. Made all changes but suggestion to do a spin-wait.

        Summary
        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.
        https://issues.apache.org/jira/browse/hbase-4507

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b
        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing
        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 23:52:43.167788) Review request for hbase. Changes ------- Address Ted's feedback. Made all changes but suggestion to do a spin-wait. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs (updated) src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2202
        -----------------------------------------------------------

        Ship it!

        Good stuff. I'm not crazy about the +1 ms but I think it's harmless (we'll literally never run into this for the META use case, as I've understood it)

        • Jonathan

        On 2011-09-29 23:52:43, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 23:52:43)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2202 ----------------------------------------------------------- Ship it! Good stuff. I'm not crazy about the +1 ms but I think it's harmless (we'll literally never run into this for the META use case, as I've understood it) Jonathan On 2011-09-29 23:52:43, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 23:52:43) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-29 22:18:31, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1948

        > <https://reviews.apache.org/r/2118/diff/1/?file=46471#file46471line1948>

        >

        > Since expectedTimestampOfLatestVersion would only be used for use case from HBASE04497, I think we just need a reliable way of enforcing this 1 ms advance.

        > Maybe spinlocking till EnvironmentEdgeManager.currentTimeMillis() returns now+1 ?

        Why do we need to wait until currentTimeMillis() rolls?

        This is pretty ugly but I think we'll need to do something for HBASE-4497 and this will work (as I'm understanding it).

        But if we didn't do the +1, wouldn't the only time this would be an issue be if RS1 was hosting R1, it then got unassigned and reassigned BACK to RS1 again, and RS1 updates META with the new location of R1 in the same millisecond that it did the original update to META the last time it opened R1. That seems overwhelmingly impossible I'd be okay without the +1 business, but at the least, let's make sure it's documented in the top-level API javadoc (i think it is)

        • Jonathan

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2185
        -----------------------------------------------------------

        On 2011-09-29 23:52:43, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 23:52:43)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-29 22:18:31, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1948 > < https://reviews.apache.org/r/2118/diff/1/?file=46471#file46471line1948 > > > Since expectedTimestampOfLatestVersion would only be used for use case from HBASE04497, I think we just need a reliable way of enforcing this 1 ms advance. > Maybe spinlocking till EnvironmentEdgeManager.currentTimeMillis() returns now+1 ? Why do we need to wait until currentTimeMillis() rolls? This is pretty ugly but I think we'll need to do something for HBASE-4497 and this will work (as I'm understanding it). But if we didn't do the +1, wouldn't the only time this would be an issue be if RS1 was hosting R1, it then got unassigned and reassigned BACK to RS1 again, and RS1 updates META with the new location of R1 in the same millisecond that it did the original update to META the last time it opened R1. That seems overwhelmingly impossible I'd be okay without the +1 business, but at the least, let's make sure it's documented in the top-level API javadoc (i think it is) Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2185 ----------------------------------------------------------- On 2011-09-29 23:52:43, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 23:52:43) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2203
        -----------------------------------------------------------

        Nice stuff, Stack.

        Some questions:

        1. The approach of "isTimestampClash and the nowBytes = Bytes.toBytes(now + 1)". It seems there is still a rare case, where EnvironmentEdgeManager.currentTimeMillis() returns expectedTimestampOfLatestVersion - 1. Then later on in put, updateKVTimestamps set it to the new now value, which happens to be expectedTimestampOfLatestVersion. Perhaps we can use "now <= expectedTimestampOfLatestVersion" condition instead of "now == expectedTimestampOfLatestVersion" in isTimestampClash. Set the new time value as nowBytes = Bytes.toBytes(expectedTimestampOfLatestVersion + 1).
        2. Do we need to modify coprocessor interfaces preCheckAndPut, postCheckAndPut, etc.? Perhaps we don't have any scenario for this yet.
        3. Do we need the same thing for checkAndDelete? Perhaps we don't have any scenario for this yet.

        • Ming

        On 2011-09-29 23:52:43, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 23:52:43)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2203 ----------------------------------------------------------- Nice stuff, Stack. Some questions: 1. The approach of "isTimestampClash and the nowBytes = Bytes.toBytes(now + 1)". It seems there is still a rare case, where EnvironmentEdgeManager.currentTimeMillis() returns expectedTimestampOfLatestVersion - 1. Then later on in put, updateKVTimestamps set it to the new now value, which happens to be expectedTimestampOfLatestVersion. Perhaps we can use "now <= expectedTimestampOfLatestVersion" condition instead of "now == expectedTimestampOfLatestVersion" in isTimestampClash. Set the new time value as nowBytes = Bytes.toBytes(expectedTimestampOfLatestVersion + 1). 2. Do we need to modify coprocessor interfaces preCheckAndPut, postCheckAndPut, etc.? Perhaps we don't have any scenario for this yet. 3. Do we need the same thing for checkAndDelete? Perhaps we don't have any scenario for this yet. Ming On 2011-09-29 23:52:43, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 23:52:43) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-30 01:19:20, Jonathan Gray wrote:

        > Good stuff. I'm not crazy about the +1 ms but I think it's harmless (we'll literally never run into this for the META use case, as I've understood it)

        Thanks boss.

        • Michael

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2202
        -----------------------------------------------------------

        On 2011-09-29 23:52:43, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 23:52:43)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-30 01:19:20, Jonathan Gray wrote: > Good stuff. I'm not crazy about the +1 ms but I think it's harmless (we'll literally never run into this for the META use case, as I've understood it) Thanks boss. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2202 ----------------------------------------------------------- On 2011-09-29 23:52:43, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 23:52:43) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-29 22:18:31, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1948

        > <https://reviews.apache.org/r/2118/diff/1/?file=46471#file46471line1948>

        >

        > Since expectedTimestampOfLatestVersion would only be used for use case from HBASE04497, I think we just need a reliable way of enforcing this 1 ms advance.

        > Maybe spinlocking till EnvironmentEdgeManager.currentTimeMillis() returns now+1 ?

        Jonathan Gray wrote:

        Why do we need to wait until currentTimeMillis() rolls?

        This is pretty ugly but I think we'll need to do something for HBASE-4497 and this will work (as I'm understanding it).

        But if we didn't do the +1, wouldn't the only time this would be an issue be if RS1 was hosting R1, it then got unassigned and reassigned BACK to RS1 again, and RS1 updates META with the new location of R1 in the same millisecond that it did the original update to META the last time it opened R1. That seems overwhelmingly impossible I'd be okay without the +1 business, but at the least, let's make sure it's documented in the top-level API javadoc (i think it is)

        I'm thinking that we should never need it but in case we do......

        Two regionservers would have to be trying to update meta at the same time AND somehow the host that was carrying .META. had its clock go backwards – e.g. .META. moved to a server whose clock was behind – and then the update of location by one of the regionservers would have to arrive at EXACTLY the millisecond the original edit had AND the update that is going in would have to be the same value as is already there.

        I think its impossible but Ming postulated it so for peace of mind, we'll make sure every edit IF the value being updated has same coordinates for sure has its on timestamp... we'll +1 it if they happen to land in same millisecond (Yes its up in the javadoc that each edit has for sure unique timestamp).

        Ted, you +1 or you have a better idea for the +1'ing (The +1'ing has a unit test!)

        • Michael

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2185
        -----------------------------------------------------------

        On 2011-09-29 23:52:43, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 23:52:43)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-29 22:18:31, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1948 > < https://reviews.apache.org/r/2118/diff/1/?file=46471#file46471line1948 > > > Since expectedTimestampOfLatestVersion would only be used for use case from HBASE04497, I think we just need a reliable way of enforcing this 1 ms advance. > Maybe spinlocking till EnvironmentEdgeManager.currentTimeMillis() returns now+1 ? Jonathan Gray wrote: Why do we need to wait until currentTimeMillis() rolls? This is pretty ugly but I think we'll need to do something for HBASE-4497 and this will work (as I'm understanding it). But if we didn't do the +1, wouldn't the only time this would be an issue be if RS1 was hosting R1, it then got unassigned and reassigned BACK to RS1 again, and RS1 updates META with the new location of R1 in the same millisecond that it did the original update to META the last time it opened R1. That seems overwhelmingly impossible I'd be okay without the +1 business, but at the least, let's make sure it's documented in the top-level API javadoc (i think it is) I'm thinking that we should never need it but in case we do...... Two regionservers would have to be trying to update meta at the same time AND somehow the host that was carrying .META. had its clock go backwards – e.g. .META. moved to a server whose clock was behind – and then the update of location by one of the regionservers would have to arrive at EXACTLY the millisecond the original edit had AND the update that is going in would have to be the same value as is already there. I think its impossible but Ming postulated it so for peace of mind, we'll make sure every edit IF the value being updated has same coordinates for sure has its on timestamp... we'll +1 it if they happen to land in same millisecond (Yes its up in the javadoc that each edit has for sure unique timestamp). Ted, you +1 or you have a better idea for the +1'ing (The +1'ing has a unit test!) Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2185 ----------------------------------------------------------- On 2011-09-29 23:52:43, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 23:52:43) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-30 01:26:24, Ming Ma wrote:

        > Nice stuff, Stack.

        >

        > Some questions:

        >

        > 1. The approach of "isTimestampClash and the nowBytes = Bytes.toBytes(now + 1)". It seems there is still a rare case, where EnvironmentEdgeManager.currentTimeMillis() returns expectedTimestampOfLatestVersion - 1. Then later on in put, updateKVTimestamps set it to the new now value, which happens to be expectedTimestampOfLatestVersion. Perhaps we can use "now <= expectedTimestampOfLatestVersion" condition instead of "now == expectedTimestampOfLatestVersion" in isTimestampClash. Set the new time value as nowBytes = Bytes.toBytes(expectedTimestampOfLatestVersion + 1).

        > 2. Do we need to modify coprocessor interfaces preCheckAndPut, postCheckAndPut, etc.? Perhaps we don't have any scenario for this yet.

        > 3. Do we need the same thing for checkAndDelete? Perhaps we don't have any scenario for this yet.

        1. Doesn't updateKVTimestamps only mess with timestamps if the timestamp is set to HConstants.LATEST_TIMESTAMP? My setting the timestamp here before we go down into internalPut means the later updateKVTimestamps will not change the timestamps I've set? Let me check for sure.
        2. Let me look into this.
        3. I considered it but stopped myself. Figured I'd let someone ask for it before I went and implemented new functionality.

        Thanks Ming for review.

        • Michael

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2203
        -----------------------------------------------------------

        On 2011-09-29 23:52:43, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 23:52:43)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-30 01:26:24, Ming Ma wrote: > Nice stuff, Stack. > > Some questions: > > 1. The approach of "isTimestampClash and the nowBytes = Bytes.toBytes(now + 1)". It seems there is still a rare case, where EnvironmentEdgeManager.currentTimeMillis() returns expectedTimestampOfLatestVersion - 1. Then later on in put, updateKVTimestamps set it to the new now value, which happens to be expectedTimestampOfLatestVersion. Perhaps we can use "now <= expectedTimestampOfLatestVersion" condition instead of "now == expectedTimestampOfLatestVersion" in isTimestampClash. Set the new time value as nowBytes = Bytes.toBytes(expectedTimestampOfLatestVersion + 1). > 2. Do we need to modify coprocessor interfaces preCheckAndPut, postCheckAndPut, etc.? Perhaps we don't have any scenario for this yet. > 3. Do we need the same thing for checkAndDelete? Perhaps we don't have any scenario for this yet. 1. Doesn't updateKVTimestamps only mess with timestamps if the timestamp is set to HConstants.LATEST_TIMESTAMP? My setting the timestamp here before we go down into internalPut means the later updateKVTimestamps will not change the timestamps I've set? Let me check for sure. 2. Let me look into this. 3. I considered it but stopped myself. Figured I'd let someone ask for it before I went and implemented new functionality. Thanks Ming for review. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2203 ----------------------------------------------------------- On 2011-09-29 23:52:43, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 23:52:43) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2215
        -----------------------------------------------------------

        Ship it!

        Please replace checkAndSet with checkAndPut in javadoc

        • Ted

        On 2011-09-29 23:52:43, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 23:52:43)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2215 ----------------------------------------------------------- Ship it! Please replace checkAndSet with checkAndPut in javadoc Ted On 2011-09-29 23:52:43, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 23:52:43) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-30 01:26:24, Ming Ma wrote:

        > Nice stuff, Stack.

        >

        > Some questions:

        >

        > 1. The approach of "isTimestampClash and the nowBytes = Bytes.toBytes(now + 1)". It seems there is still a rare case, where EnvironmentEdgeManager.currentTimeMillis() returns expectedTimestampOfLatestVersion - 1. Then later on in put, updateKVTimestamps set it to the new now value, which happens to be expectedTimestampOfLatestVersion. Perhaps we can use "now <= expectedTimestampOfLatestVersion" condition instead of "now == expectedTimestampOfLatestVersion" in isTimestampClash. Set the new time value as nowBytes = Bytes.toBytes(expectedTimestampOfLatestVersion + 1).

        > 2. Do we need to modify coprocessor interfaces preCheckAndPut, postCheckAndPut, etc.? Perhaps we don't have any scenario for this yet.

        > 3. Do we need the same thing for checkAndDelete? Perhaps we don't have any scenario for this yet.

        Michael Stack wrote:

        1. Doesn't updateKVTimestamps only mess with timestamps if the timestamp is set to HConstants.LATEST_TIMESTAMP? My setting the timestamp here before we go down into internalPut means the later updateKVTimestamps will not change the timestamps I've set? Let me check for sure.

        2. Let me look into this.

        3. I considered it but stopped myself. Figured I'd let someone ask for it before I went and implemented new functionality.

        Thanks Ming for review.

        I took another look and now I see what you are saying Ming. New patch coming.

        For 1., for the case where we are all up in the same millisecond, then I'll pass the 'now' to the innerPut so that when it does its updateKVTimestamps, its operating with the same 'now'.

        I fixed 2. adding this new method to RegionObserver and handling the ripple through the code base (Man, its harder now adding new methods to a regionserver!)

        I'm not going to do 3. in scope of this issue.

        @Ted New patch will include your suggestion too.

        Thanks for review lads.

        • Michael

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2203
        -----------------------------------------------------------

        On 2011-09-29 23:52:43, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-29 23:52:43)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-30 01:26:24, Ming Ma wrote: > Nice stuff, Stack. > > Some questions: > > 1. The approach of "isTimestampClash and the nowBytes = Bytes.toBytes(now + 1)". It seems there is still a rare case, where EnvironmentEdgeManager.currentTimeMillis() returns expectedTimestampOfLatestVersion - 1. Then later on in put, updateKVTimestamps set it to the new now value, which happens to be expectedTimestampOfLatestVersion. Perhaps we can use "now <= expectedTimestampOfLatestVersion" condition instead of "now == expectedTimestampOfLatestVersion" in isTimestampClash. Set the new time value as nowBytes = Bytes.toBytes(expectedTimestampOfLatestVersion + 1). > 2. Do we need to modify coprocessor interfaces preCheckAndPut, postCheckAndPut, etc.? Perhaps we don't have any scenario for this yet. > 3. Do we need the same thing for checkAndDelete? Perhaps we don't have any scenario for this yet. Michael Stack wrote: 1. Doesn't updateKVTimestamps only mess with timestamps if the timestamp is set to HConstants.LATEST_TIMESTAMP? My setting the timestamp here before we go down into internalPut means the later updateKVTimestamps will not change the timestamps I've set? Let me check for sure. 2. Let me look into this. 3. I considered it but stopped myself. Figured I'd let someone ask for it before I went and implemented new functionality. Thanks Ming for review. I took another look and now I see what you are saying Ming. New patch coming. For 1., for the case where we are all up in the same millisecond, then I'll pass the 'now' to the innerPut so that when it does its updateKVTimestamps, its operating with the same 'now'. I fixed 2. adding this new method to RegionObserver and handling the ripple through the code base (Man, its harder now adding new methods to a regionserver!) I'm not going to do 3. in scope of this issue. @Ted New patch will include your suggestion too. Thanks for review lads. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2203 ----------------------------------------------------------- On 2011-09-29 23:52:43, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-29 23:52:43) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/
        -----------------------------------------------------------

        (Updated 2011-09-30 21:03:05.270519)

        Review request for hbase.

        Changes
        -------

        This version fixes Teds issue and then it does 1. and 2. in Ming's list of 3.

        I refactored some moving the timestamp messing down into innerPut. This cleans up the patch significantly.

        Added more javadoc on the method to the effect that this method is for low-frequency updates on a cell only; i.e. not > one edit per millisecond.

        What you think Ming?

        Summary
        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.
        https://issues.apache.org/jira/browse/hbase-4507

        Diffs (updated)


        src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 96429c7
        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b
        src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 35377a9
        src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 20381f2
        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c
        src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java b05aef2

        Diff: https://reviews.apache.org/r/2118/diff

        Testing
        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-30 21:03:05.270519) Review request for hbase. Changes ------- This version fixes Teds issue and then it does 1. and 2. in Ming's list of 3. I refactored some moving the timestamp messing down into innerPut. This cleans up the patch significantly. Added more javadoc on the method to the effect that this method is for low-frequency updates on a cell only; i.e. not > one edit per millisecond. What you think Ming? Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs (updated) src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 96429c7 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 35377a9 src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 20381f2 src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java b05aef2 Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        Todd Lipcon added a comment -

        Do we really need to modify checkAndPut here? I think it's safer to have explicit sequence IDs for the META rows and use the existing checkAndPut. Unless there's some reason that won't work, I don't think it's necessary to change a core function for this?

        Show
        Todd Lipcon added a comment - Do we really need to modify checkAndPut here? I think it's safer to have explicit sequence IDs for the META rows and use the existing checkAndPut. Unless there's some reason that won't work, I don't think it's necessary to change a core function for this?
        Hide
        Todd Lipcon added a comment -

        If we're going to add more functionality to checkAndPut I'd really rather do it with a clean API, like:

        boolean atomicMutation(RowPredicates predicates, Action mutation);
        

        used something like:

        RowPredicates pred = new RowPredicates.Builder()
          .matchTimestamp(ComparisonOp.EQUALS, 12345)
          .matchCellValue(colFam, qual, ComparisonOp.EQUALS, val)
          .build();
        Action mutation = new Put(...);
        table.atomicMutation(pred, mutation);
        

        so we don't have to add a new checkAndPut API for every combinatoric set of options and mutations

        Show
        Todd Lipcon added a comment - If we're going to add more functionality to checkAndPut I'd really rather do it with a clean API, like: boolean atomicMutation(RowPredicates predicates, Action mutation); used something like: RowPredicates pred = new RowPredicates.Builder() .matchTimestamp(ComparisonOp.EQUALS, 12345) .matchCellValue(colFam, qual, ComparisonOp.EQUALS, val) .build(); Action mutation = new Put(...); table.atomicMutation(pred, mutation); so we don't have to add a new checkAndPut API for every combinatoric set of options and mutations
        Hide
        Todd Lipcon added a comment -

        (actually, reusing Filter for this may make sense)

        Show
        Todd Lipcon added a comment - (actually, reusing Filter for this may make sense)
        Hide
        stack added a comment -

        Do we really need to modify checkAndPut here?

        checkAndPut as is has a hole in that an update that writes the same value will go unnoticed.

        I think it's safer to have explicit sequence IDs for the META rows and use the existing checkAndPut. Unless there's some reason that won't work, I don't think it's necessary to change a core function for this?

        So, each time we set a region location (hostname + port + startcode), we'd also update a new column, say info:editid, which would have a uuid value. We'd then do checkAndPut on the info:editid column?

        We'd still need to read whatever this value was at start of region open.

        I could work up a patch for this over in hbase-4497 if its preferred.

        Regards atomicMutation, that would be a nice improvement. Would deprecate current APIs in favor of it going forward. Lets open a new issue to do that.

        Show
        stack added a comment - Do we really need to modify checkAndPut here? checkAndPut as is has a hole in that an update that writes the same value will go unnoticed. I think it's safer to have explicit sequence IDs for the META rows and use the existing checkAndPut. Unless there's some reason that won't work, I don't think it's necessary to change a core function for this? So, each time we set a region location (hostname + port + startcode), we'd also update a new column, say info:editid, which would have a uuid value. We'd then do checkAndPut on the info:editid column? We'd still need to read whatever this value was at start of region open. I could work up a patch for this over in hbase-4497 if its preferred. Regards atomicMutation, that would be a nice improvement. Would deprecate current APIs in favor of it going forward. Lets open a new issue to do that.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2118/#review2238
        -----------------------------------------------------------

        Ship it!

        It looks good. BTW, it looks like we don't have unit tests for RegionObserver.preCheckAndPut and RegionObserver.postCheckAndPut. Maybe need a separate jira for that.

        • Ming

        On 2011-09-30 21:03:05, Michael Stack wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2118/

        -----------------------------------------------------------

        (Updated 2011-09-30 21:03:05)

        Review request for hbase.

        Summary

        -------

        Adds a checkAndPut that takes a timestamp

        This addresses bug hbase-4507.

        https://issues.apache.org/jira/browse/hbase-4507

        Diffs

        -----

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 96429c7

        src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b

        src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 35377a9

        src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 20381f2

        src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c

        src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java b05aef2

        Diff: https://reviews.apache.org/r/2118/diff

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/#review2238 ----------------------------------------------------------- Ship it! It looks good. BTW, it looks like we don't have unit tests for RegionObserver.preCheckAndPut and RegionObserver.postCheckAndPut. Maybe need a separate jira for that. Ming On 2011-09-30 21:03:05, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2118/ ----------------------------------------------------------- (Updated 2011-09-30 21:03:05) Review request for hbase. Summary ------- Adds a checkAndPut that takes a timestamp This addresses bug hbase-4507. https://issues.apache.org/jira/browse/hbase-4507 Diffs ----- src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 96429c7 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 99b34cc src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 35377a9 src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 20381f2 src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 3679c02 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 6ec857c src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java b05aef2 Diff: https://reviews.apache.org/r/2118/diff Testing ------- Thanks, Michael
        Hide
        Todd Lipcon added a comment -

        So, each time we set a region location (hostname + port + startcode), we'd also update a new column, say info:editid, which would have a uuid value. We'd then do checkAndPut on the info:editid column?

        Yep, exactly. With this method we wouldn't have to change/add APIs here...

        We'd still need to read whatever this value was at start of region open.

        Right, but you have to do that with the timestamp method, too.

        Show
        Todd Lipcon added a comment - So, each time we set a region location (hostname + port + startcode), we'd also update a new column, say info:editid, which would have a uuid value. We'd then do checkAndPut on the info:editid column? Yep, exactly. With this method we wouldn't have to change/add APIs here... We'd still need to read whatever this value was at start of region open. Right, but you have to do that with the timestamp method, too.
        Hide
        stack added a comment -

        @Todd Ok. I'll work up a patch over in hbase-4497. For this issue, you against committing whats here? And want me to open new issue for the refactor you suggest above?

        Show
        stack added a comment - @Todd Ok. I'll work up a patch over in hbase-4497. For this issue, you against committing whats here? And want me to open new issue for the refactor you suggest above?
        Hide
        Todd Lipcon added a comment -

        Yea, I don't want to introduce a new API here. Having taught an HBase training last week, people found our abundance of non-orthogonal APIs rather confusing. If we add a public API, we have to maintain it forever (look at the mess of getRowClosestAtOrBefore for example). So I'm -1 on committing a new public API when we don't have a compelling use case not satisfied by existing APIs... what's wrong with using a cell?

        Show
        Todd Lipcon added a comment - Yea, I don't want to introduce a new API here. Having taught an HBase training last week, people found our abundance of non-orthogonal APIs rather confusing. If we add a public API, we have to maintain it forever (look at the mess of getRowClosestAtOrBefore for example). So I'm -1 on committing a new public API when we don't have a compelling use case not satisfied by existing APIs... what's wrong with using a cell?
        Hide
        stack added a comment -

        Ok. I buy that.

        Show
        stack added a comment - Ok. I buy that.
        Hide
        stack added a comment -

        Though... (bicycle ride between above comment and this)... its kinda lame our versioning can't be used detecting a cell has been changed. Our versioning has the above described hole where an update that doesn't change the value goes undetected (without this hack applied). This is kinda lame. Let me open an issue to address this. This patch did some clog dancing to ensure each edit had its own 'version' timestamp but has limitations (only one edit per ms max). Its like we need a edit sequence id per millisecond. Let me open a new issue to address this shortfall properly.

        Show
        stack added a comment - Though... (bicycle ride between above comment and this)... its kinda lame our versioning can't be used detecting a cell has been changed. Our versioning has the above described hole where an update that doesn't change the value goes undetected (without this hack applied). This is kinda lame. Let me open an issue to address this. This patch did some clog dancing to ensure each edit had its own 'version' timestamp but has limitations (only one edit per ms max). Its like we need a edit sequence id per millisecond. Let me open a new issue to address this shortfall properly.
        Hide
        stack added a comment -

        I opened the wish issue HBASE-4527

        Show
        stack added a comment - I opened the wish issue HBASE-4527
        Hide
        stack added a comment -

        Resolving a -1'd issue as won't fix (HBASE-4527 takes over)

        Show
        stack added a comment - Resolving a -1'd issue as won't fix ( HBASE-4527 takes over)

          People

          • Assignee:
            stack
            Reporter:
            Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development