[jira] [Commented] (HBASE-2425) Crossport HADOOP-1849 rpc fix
[ https://issues.apache.org/jira/browse/HBASE-2425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13144218#comment-13144218 ] jirapos...@reviews.apache.org commented on HBASE-2425: -- --- 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: bq. bq. --- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/2718/ bq. --- bq. bq. (Updated 2011-11-04 00:11:21) bq. bq. bq. Review request for hbase. bq. bq. bq. Summary bq. --- bq. bq. Versions of Gary suggestions bq. bq. bq. This addresses bug hbase-2425. bq. https://issues.apache.org/jira/browse/hbase-2425 bq. bq. bq. Diffs bq. - bq. bq. src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 bq.src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f bq. src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 bq.src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 bq.src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 bq.src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a bq.src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 bq.src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION bq.src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION bq.src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 bq.src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 bq.src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 bq.src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 bq.src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e bq.src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 bq. bq. Diff: https://reviews.apache.org/r/2718/diff bq. bq. bq. Testing bq. --- bq. bq. bq. Thanks, bq. bq. Michael bq. bq. Crossport HADOOP-1849 rpc fix - Key: HBASE-2425 URL: https://issues.apache.org/jira/browse/HBASE-2425 Project: HBase Issue Type: Task Reporter: stack Labels: moved_from_0_20_5 Suggested over in HBASE-2360. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-2425) Crossport HADOOP-1849 rpc fix
[ https://issues.apache.org/jira/browse/HBASE-2425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13144228#comment-13144228 ] jirapos...@reviews.apache.org commented on HBASE-2425: -- bq. On 2011-11-04 18:06:38, Gary Helmling wrote: bq. Just a couple of comments. Otherwise looks good to me. Excellent. Thanks for the review. bq. On 2011-11-04 18:06:38, Gary Helmling wrote: bq. src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 322 bq. https://reviews.apache.org/r/2718/diff/1/?file=56222#file56222line322 bq. bq. 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). bq. On 2011-11-04 18:06:38, Gary Helmling wrote: bq. src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java, line 98 bq. https://reviews.apache.org/r/2718/diff/1/?file=56223#file56223line98 bq. bq. 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. bq. On 2011-11-04 18:06:38, Gary Helmling wrote: bq. src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java, line 116 bq. https://reviews.apache.org/r/2718/diff/1/?file=56223#file56223line116 bq. bq. Probably better to use super.write(out) here. Same code, but future proof to changes. Ditto. bq. On 2011-11-04 18:06:38, Gary Helmling wrote: bq. src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java, line 60 bq. https://reviews.apache.org/r/2718/diff/1/?file=56227#file56227line60 bq. bq. Should be able to remove this now that Invocation implements VersionedWritable. Agreed. bq. On 2011-11-04 18:06:38, Gary Helmling wrote: bq. src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java, line 343 bq. https://reviews.apache.org/r/2718/diff/1/?file=56227#file56227line343 bq. bq. 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: bq. bq. --- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/2718/ bq. --- bq. bq. (Updated 2011-11-04 00:11:21) bq. bq. bq. Review request for hbase. bq. bq. bq. Summary bq. --- bq. bq. Versions of Gary suggestions bq. bq. bq. This addresses bug hbase-2425. bq. https://issues.apache.org/jira/browse/hbase-2425 bq. bq. bq. Diffs bq. - bq. bq. src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 bq.src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f bq. src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 bq.src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 bq.src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 bq.src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a bq.src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 bq.src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION bq.src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION bq.src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 bq.src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 bq.src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 bq.src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 bq.src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e bq.src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 bq. bq. Diff: https://reviews.apache.org/r/2718/diff bq. bq. bq. Testing bq. --- bq. bq. bq. Thanks, bq. bq. Michael bq. bq. Crossport HADOOP-1849 rpc fix - Key: HBASE-2425 URL: https://issues.apache.org/jira/browse/HBASE-2425 Project: HBase Issue Type:
[jira] [Commented] (HBASE-2425) Crossport HADOOP-1849 rpc fix
[ https://issues.apache.org/jira/browse/HBASE-2425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13144239#comment-13144239 ] jirapos...@reviews.apache.org commented on HBASE-2425: -- bq. On 2011-11-04 18:06:38, Gary Helmling wrote: bq. src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 322 bq. https://reviews.apache.org/r/2718/diff/1/?file=56222#file56222line322 bq. bq. 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? bq. bq. Michael Stack wrote: bq. 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: bq. bq. --- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/2718/ bq. --- bq. bq. (Updated 2011-11-04 00:11:21) bq. bq. bq. Review request for hbase. bq. bq. bq. Summary bq. --- bq. bq. Versions of Gary suggestions bq. bq. bq. This addresses bug hbase-2425. bq. https://issues.apache.org/jira/browse/hbase-2425 bq. bq. bq. Diffs bq. - bq. bq. src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 bq.src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f bq. src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 bq.src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 bq.src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 bq.src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a bq.src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 bq.src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION bq.src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION bq.src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 bq.src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 bq.src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 bq.src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 bq.src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e bq.src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 bq. bq. Diff: https://reviews.apache.org/r/2718/diff bq. bq. bq. Testing bq. --- bq. bq. bq. Thanks, bq. bq. Michael bq. bq. Crossport HADOOP-1849 rpc fix - Key: HBASE-2425 URL: https://issues.apache.org/jira/browse/HBASE-2425 Project: HBase Issue Type: Task Reporter: stack Labels: moved_from_0_20_5 Suggested over in HBASE-2360. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-2425) Crossport HADOOP-1849 rpc fix
[ https://issues.apache.org/jira/browse/HBASE-2425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13144348#comment-13144348 ] jirapos...@reviews.apache.org commented on HBASE-2425: -- bq. On 2011-11-04 18:06:38, Gary Helmling wrote: bq. src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 322 bq. https://reviews.apache.org/r/2718/diff/1/?file=56222#file56222line322 bq. bq. 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? bq. bq. Michael Stack wrote: bq. Its for asynchbase -- so it can tell diff between a response with length and one w/o (its trying to support all hbase versions). bq. bq. Gary Helmling wrote: bq. 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: bq. bq. --- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/2718/ bq. --- bq. bq. (Updated 2011-11-04 00:11:21) bq. bq. bq. Review request for hbase. bq. bq. bq. Summary bq. --- bq. bq. Versions of Gary suggestions bq. bq. bq. This addresses bug hbase-2425. bq. https://issues.apache.org/jira/browse/hbase-2425 bq. bq. bq. Diffs bq. - bq. bq. src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 bq.src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f bq. src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 bq.src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 bq.src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 bq.src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a bq.src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 bq.src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION bq.src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION bq.src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 bq.src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 bq.src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 bq.src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 bq.src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e bq.src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 bq. bq. Diff: https://reviews.apache.org/r/2718/diff bq. bq. bq. Testing bq. --- bq. bq. bq. Thanks, bq. bq. Michael bq. bq. Crossport HADOOP-1849 rpc fix - Key: HBASE-2425 URL: https://issues.apache.org/jira/browse/HBASE-2425 Project: HBase Issue Type: Task Reporter: stack Labels: moved_from_0_20_5 Suggested over in HBASE-2360. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-2425) Crossport HADOOP-1849 rpc fix
[ https://issues.apache.org/jira/browse/HBASE-2425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13144567#comment-13144567 ] jirapos...@reviews.apache.org commented on HBASE-2425: -- bq. On 2011-11-04 18:06:38, Gary Helmling wrote: bq. src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java, line 60 bq. https://reviews.apache.org/r/2718/diff/1/?file=56227#file56227line60 bq. bq. Should be able to remove this now that Invocation implements VersionedWritable. bq. bq. Michael Stack wrote: bq. 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: bq. bq. --- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/2718/ bq. --- bq. bq. (Updated 2011-11-04 00:11:21) bq. bq. bq. Review request for hbase. bq. bq. bq. Summary bq. --- bq. bq. Versions of Gary suggestions bq. bq. bq. This addresses bug hbase-2425. bq. https://issues.apache.org/jira/browse/hbase-2425 bq. bq. bq. Diffs bq. - bq. bq. src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java fce5490 bq.src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateProtocol.java 2fa4d6f bq. src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java 6f88357 bq.src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java 6fcb771 bq.src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1365411 bq.src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 4a8918a bq.src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java e60f970 bq.src/main/java/org/apache/hadoop/hbase/ipc/ProtocolSignature.java PRE-CREATION bq.src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION bq.src/main/java/org/apache/hadoop/hbase/ipc/VersionedProtocol.java fb07374 bq.src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 60a9248 bq.src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 8de2314 bq.src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d0e4c5 bq.src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 12bd33e bq.src/test/java/org/apache/hadoop/hbase/ipc/TestDelayedRpc.java 888f428 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e5b6a78 bq. bq. Diff: https://reviews.apache.org/r/2718/diff bq. bq. bq. Testing bq. --- bq. bq. bq. Thanks, bq. bq. Michael bq. bq. Crossport HADOOP-1849 rpc fix - Key: HBASE-2425 URL: https://issues.apache.org/jira/browse/HBASE-2425 Project: HBase Issue Type: Task Reporter: stack Labels: moved_from_0_20_5 Suggested over in HBASE-2360. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-2425) Crossport HADOOP-1849 rpc fix
[ https://issues.apache.org/jira/browse/HBASE-2425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13143639#comment-13143639 ] jirapos...@reviews.apache.org commented on HBASE-2425: -- --- 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 Crossport HADOOP-1849 rpc fix - Key: HBASE-2425 URL: https://issues.apache.org/jira/browse/HBASE-2425 Project: HBase Issue Type: Task Reporter: stack Labels: moved_from_0_20_5 Suggested over in HBASE-2360. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
