Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Hadoop Flags:
      Reviewed

      Description

      Suggested over in HBASE-2360.

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          We should also still consider whether we can just move back to using hadoop IPC directly

          Show
          Todd Lipcon added a comment - We should also still consider whether we can just move back to using hadoop IPC directly
          Hide
          stack added a comment -

          Yes. I do not think there obstacles to our doing that any more (Would be post 0.20.4 because we need patch that swapped back to using method name Strings instead of codes in rpc).

          Show
          stack added a comment - Yes. I do not think there obstacles to our doing that any more (Would be post 0.20.4 because we need patch that swapped back to using method name Strings instead of codes in rpc).
          Hide
          stack added a comment -

          Bulk move of 0.20.5 issues into 0.21.0 after vote that we merge branch into TRUNK up on list.

          Show
          stack added a comment - Bulk move of 0.20.5 issues into 0.21.0 after vote that we merge branch into TRUNK up on list.
          Hide
          stack added a comment -

          Moving out. Our RPC has changed a bunch and Gary is making RPC pluggable. Won't be done for 0.90.x Moving out.

          Show
          stack added a comment - Moving out. Our RPC has changed a bunch and Gary is making RPC pluggable. Won't be done for 0.90.x Moving out.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for hbase.

          Summary
          -------

          Versions of Gary suggestions

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

          Diffs


          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490
          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357
          src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a
          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970
          src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248
          src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314
          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e
          src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428
          src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78

          Diff: https://reviews.apache.org/r/2718/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/2718/ ----------------------------------------------------------- Review request for hbase. Summary ------- Versions of Gary suggestions This addresses bug hbase-2425. https://issues.apache.org/jira/browse/hbase-2425 Diffs src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 Diff: https://reviews.apache.org/r/2718/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/2718/#review3049
          -----------------------------------------------------------

          Just a couple of comments. Otherwise looks good to me.

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
          <https://reviews.apache.org/r/2718/#comment6797>

          We could eliminate the flag and use status instead. Are there plans for other bits being set in this? Otherwise, we always have length and error can be determined from Status. Or would removing this break asynchbase in other ways?

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
          <https://reviews.apache.org/r/2718/#comment6796>

          Good

          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
          <https://reviews.apache.org/r/2718/#comment6792>

          Don't think this is necessary? The super.readFields(in) should throw VersionMismatchException if the read version doesn't match our getVersion().

          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
          <https://reviews.apache.org/r/2718/#comment6793>

          Probably better to use super.write(out) here. Same code, but future proof to changes.

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          <https://reviews.apache.org/r/2718/#comment6794>

          Should be able to remove this now that Invocation implements VersionedWritable.

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          <https://reviews.apache.org/r/2718/#comment6795>

          Should be able to remove this now that Invocation implements VersionedWritable. I didn't see any dependency on rpc version outside of the Invocation serialization.

          • Gary

          On 2011-11-04 00:11:21, Michael Stack wrote:

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

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

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

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

          (Updated 2011-11-04 00:11:21)

          Review request for hbase.

          Summary

          -------

          Versions of Gary suggestions

          This addresses bug hbase-2425.

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

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490

          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357

          src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a

          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970

          src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION

          src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION

          src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248

          src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e

          src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428

          src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78

          Diff: https://reviews.apache.org/r/2718/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/2718/#review3049 ----------------------------------------------------------- Just a couple of comments. Otherwise looks good to me. src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/2718/#comment6797 > We could eliminate the flag and use status instead. Are there plans for other bits being set in this? Otherwise, we always have length and error can be determined from Status. Or would removing this break asynchbase in other ways? src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/2718/#comment6796 > Good src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java < https://reviews.apache.org/r/2718/#comment6792 > Don't think this is necessary? The super.readFields(in) should throw VersionMismatchException if the read version doesn't match our getVersion(). src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java < https://reviews.apache.org/r/2718/#comment6793 > Probably better to use super.write(out) here. Same code, but future proof to changes. src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java < https://reviews.apache.org/r/2718/#comment6794 > Should be able to remove this now that Invocation implements VersionedWritable. src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java < https://reviews.apache.org/r/2718/#comment6795 > Should be able to remove this now that Invocation implements VersionedWritable. I didn't see any dependency on rpc version outside of the Invocation serialization. Gary On 2011-11-04 00:11:21, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2718/ ----------------------------------------------------------- (Updated 2011-11-04 00:11:21) Review request for hbase. Summary ------- Versions of Gary suggestions This addresses bug hbase-2425. https://issues.apache.org/jira/browse/hbase-2425 Diffs ----- src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 Diff: https://reviews.apache.org/r/2718/diff Testing ------- Thanks, Michael
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-04 18:06:38, Gary Helmling wrote:

          > Just a couple of comments. Otherwise looks good to me.

          Excellent. Thanks for the review.

          On 2011-11-04 18:06:38, Gary Helmling wrote:

          > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 322

          > <https://reviews.apache.org/r/2718/diff/1/?file=56222#file56222line322>

          >

          > We could eliminate the flag and use status instead. Are there plans for other bits being set in this? Otherwise, we always have length and error can be determined from Status. Or would removing this break asynchbase in other ways?

          Its for asynchbase – so it can tell diff between a response with length and one w/o (its trying to support all hbase versions).

          On 2011-11-04 18:06:38, Gary Helmling wrote:

          > src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java, line 98

          > <https://reviews.apache.org/r/2718/diff/1/?file=56223#file56223line98>

          >

          > Don't think this is necessary? The super.readFields(in) should throw VersionMismatchException if the read version doesn't match our getVersion().

          My mistake. Will fix.

          On 2011-11-04 18:06:38, Gary Helmling wrote:

          > src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java, line 116

          > <https://reviews.apache.org/r/2718/diff/1/?file=56223#file56223line116>

          >

          > Probably better to use super.write(out) here. Same code, but future proof to changes.

          Ditto.

          On 2011-11-04 18:06:38, Gary Helmling wrote:

          > src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java, line 60

          > <https://reviews.apache.org/r/2718/diff/1/?file=56227#file56227line60>

          >

          > Should be able to remove this now that Invocation implements VersionedWritable.

          Agreed.

          On 2011-11-04 18:06:38, Gary Helmling wrote:

          > src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java, line 343

          > <https://reviews.apache.org/r/2718/diff/1/?file=56227#file56227line343>

          >

          > Should be able to remove this now that Invocation implements VersionedWritable. I didn't see any dependency on rpc version outside of the Invocation serialization.

          Good.

          • Michael

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

          On 2011-11-04 00:11:21, Michael Stack wrote:

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

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

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

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

          (Updated 2011-11-04 00:11:21)

          Review request for hbase.

          Summary

          -------

          Versions of Gary suggestions

          This addresses bug hbase-2425.

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

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490

          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357

          src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a

          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970

          src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION

          src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION

          src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248

          src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e

          src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428

          src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78

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

          Testing

          -------

          Thanks,

          Michael

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-04 18:06:38, Gary Helmling wrote: > Just a couple of comments. Otherwise looks good to me. Excellent. Thanks for the review. On 2011-11-04 18:06:38, Gary Helmling wrote: > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 322 > < https://reviews.apache.org/r/2718/diff/1/?file=56222#file56222line322 > > > We could eliminate the flag and use status instead. Are there plans for other bits being set in this? Otherwise, we always have length and error can be determined from Status. Or would removing this break asynchbase in other ways? Its for asynchbase – so it can tell diff between a response with length and one w/o (its trying to support all hbase versions). On 2011-11-04 18:06:38, Gary Helmling wrote: > src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java, line 98 > < https://reviews.apache.org/r/2718/diff/1/?file=56223#file56223line98 > > > Don't think this is necessary? The super.readFields(in) should throw VersionMismatchException if the read version doesn't match our getVersion(). My mistake. Will fix. On 2011-11-04 18:06:38, Gary Helmling wrote: > src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java, line 116 > < https://reviews.apache.org/r/2718/diff/1/?file=56223#file56223line116 > > > Probably better to use super.write(out) here. Same code, but future proof to changes. Ditto. On 2011-11-04 18:06:38, Gary Helmling wrote: > src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java, line 60 > < https://reviews.apache.org/r/2718/diff/1/?file=56227#file56227line60 > > > Should be able to remove this now that Invocation implements VersionedWritable. Agreed. On 2011-11-04 18:06:38, Gary Helmling wrote: > src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java, line 343 > < https://reviews.apache.org/r/2718/diff/1/?file=56227#file56227line343 > > > Should be able to remove this now that Invocation implements VersionedWritable. I didn't see any dependency on rpc version outside of the Invocation serialization. Good. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2718/#review3049 ----------------------------------------------------------- On 2011-11-04 00:11:21, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2718/ ----------------------------------------------------------- (Updated 2011-11-04 00:11:21) Review request for hbase. Summary ------- Versions of Gary suggestions This addresses bug hbase-2425. https://issues.apache.org/jira/browse/hbase-2425 Diffs ----- src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 Diff: https://reviews.apache.org/r/2718/diff Testing ------- Thanks, Michael
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-04 18:06:38, Gary Helmling wrote:

          > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 322

          > <https://reviews.apache.org/r/2718/diff/1/?file=56222#file56222line322>

          >

          > We could eliminate the flag and use status instead. Are there plans for other bits being set in this? Otherwise, we always have length and error can be determined from Status. Or would removing this break asynchbase in other ways?

          Michael Stack wrote:

          Its for asynchbase – so it can tell diff between a response with length and one w/o (its trying to support all hbase versions).

          So do we need an additional flag for "status set" here, so that asynchbase can tell when that's included? Or can it pick that up from the version number?

          • Gary

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

          On 2011-11-04 00:11:21, Michael Stack wrote:

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

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

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

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

          (Updated 2011-11-04 00:11:21)

          Review request for hbase.

          Summary

          -------

          Versions of Gary suggestions

          This addresses bug hbase-2425.

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

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490

          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357

          src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a

          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970

          src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION

          src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION

          src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248

          src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e

          src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428

          src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78

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

          Testing

          -------

          Thanks,

          Michael

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-04 18:06:38, Gary Helmling wrote: > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 322 > < https://reviews.apache.org/r/2718/diff/1/?file=56222#file56222line322 > > > We could eliminate the flag and use status instead. Are there plans for other bits being set in this? Otherwise, we always have length and error can be determined from Status. Or would removing this break asynchbase in other ways? Michael Stack wrote: Its for asynchbase – so it can tell diff between a response with length and one w/o (its trying to support all hbase versions). So do we need an additional flag for "status set" here, so that asynchbase can tell when that's included? Or can it pick that up from the version number? Gary ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2718/#review3049 ----------------------------------------------------------- On 2011-11-04 00:11:21, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2718/ ----------------------------------------------------------- (Updated 2011-11-04 00:11:21) Review request for hbase. Summary ------- Versions of Gary suggestions This addresses bug hbase-2425. https://issues.apache.org/jira/browse/hbase-2425 Diffs ----- src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 Diff: https://reviews.apache.org/r/2718/diff Testing ------- Thanks, Michael
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-04 18:06:38, Gary Helmling wrote:

          > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 322

          > <https://reviews.apache.org/r/2718/diff/1/?file=56222#file56222line322>

          >

          > We could eliminate the flag and use status instead. Are there plans for other bits being set in this? Otherwise, we always have length and error can be determined from Status. Or would removing this break asynchbase in other ways?

          Michael Stack wrote:

          Its for asynchbase – so it can tell diff between a response with length and one w/o (its trying to support all hbase versions).

          Gary Helmling wrote:

          So do we need an additional flag for "status set" here, so that asynchbase can tell when that's included? Or can it pick that up from the version number?

          asynchbase doesn't work against 0.92 yet; therefore, the flag will mean length + status (will check w/ the B man).

          • Michael

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

          On 2011-11-04 00:11:21, Michael Stack wrote:

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

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

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

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

          (Updated 2011-11-04 00:11:21)

          Review request for hbase.

          Summary

          -------

          Versions of Gary suggestions

          This addresses bug hbase-2425.

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

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490

          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357

          src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a

          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970

          src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION

          src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION

          src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248

          src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e

          src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428

          src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78

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

          Testing

          -------

          Thanks,

          Michael

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-04 18:06:38, Gary Helmling wrote: > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 322 > < https://reviews.apache.org/r/2718/diff/1/?file=56222#file56222line322 > > > We could eliminate the flag and use status instead. Are there plans for other bits being set in this? Otherwise, we always have length and error can be determined from Status. Or would removing this break asynchbase in other ways? Michael Stack wrote: Its for asynchbase – so it can tell diff between a response with length and one w/o (its trying to support all hbase versions). Gary Helmling wrote: So do we need an additional flag for "status set" here, so that asynchbase can tell when that's included? Or can it pick that up from the version number? asynchbase doesn't work against 0.92 yet; therefore, the flag will mean length + status (will check w/ the B man). Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2718/#review3049 ----------------------------------------------------------- On 2011-11-04 00:11:21, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2718/ ----------------------------------------------------------- (Updated 2011-11-04 00:11:21) Review request for hbase. Summary ------- Versions of Gary suggestions This addresses bug hbase-2425. https://issues.apache.org/jira/browse/hbase-2425 Diffs ----- src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 Diff: https://reviews.apache.org/r/2718/diff Testing ------- Thanks, Michael
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-04 18:06:38, Gary Helmling wrote:

          > src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java, line 60

          > <https://reviews.apache.org/r/2718/diff/1/?file=56227#file56227line60>

          >

          > Should be able to remove this now that Invocation implements VersionedWritable.

          Michael Stack wrote:

          Agreed.

          oh, I see what you mean. Invocation is now versioned and will fail on deserialization.

          • Michael

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

          On 2011-11-04 00:11:21, Michael Stack wrote:

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

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

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

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

          (Updated 2011-11-04 00:11:21)

          Review request for hbase.

          Summary

          -------

          Versions of Gary suggestions

          This addresses bug hbase-2425.

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

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490

          src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357

          src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a

          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970

          src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION

          src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION

          src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248

          src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e

          src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428

          src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78

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

          Testing

          -------

          Thanks,

          Michael

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-04 18:06:38, Gary Helmling wrote: > src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java, line 60 > < https://reviews.apache.org/r/2718/diff/1/?file=56227#file56227line60 > > > Should be able to remove this now that Invocation implements VersionedWritable. Michael Stack wrote: Agreed. oh, I see what you mean. Invocation is now versioned and will fail on deserialization. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2718/#review3049 ----------------------------------------------------------- On 2011-11-04 00:11:21, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2718/ ----------------------------------------------------------- (Updated 2011-11-04 00:11:21) Review request for hbase. Summary ------- Versions of Gary suggestions This addresses bug hbase-2425. https://issues.apache.org/jira/browse/hbase-2425 Diffs ----- src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 Diff: https://reviews.apache.org/r/2718/diff Testing ------- Thanks, Michael
          Hide
          stack added a comment -

          Bulk of this looks to have been committed. Resolving.

          Show
          stack added a comment - Bulk of this looks to have been committed. Resolving.

            People

            • Assignee:
              Unassigned
              Reporter:
              stack
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development