Review Board 1.7.22


FLUME-1896. Thrift Rpc Client.

Review Request #9284 - Created Feb. 4, 2013 and submitted

Hari Shreedharan
FLUME-1896
Reviewers
Flume
flume-git
Added thrift rpc client. Detailed description posted on jira.
Added unit tests for new code.
Total:
25
Open:
20
Resolved:
0
Dropped:
5
Status:
From:
Description From Last Updated Status
If this fails we do we want to print the error and then exit? Brock Noland Feb. 7, 2013, 3:40 p.m. Open
same here? Brock Noland Feb. 7, 2013, 3:40 p.m. Open
Same questions on this script Brock Noland Feb. 7, 2013, 3:40 p.m. Open
nit: Looks like the variables in the constructor can be set to final Brock Noland Feb. 7, 2013, 3:40 p.m. Open
nit: it's nice to have this up at the top of the class Brock Noland Feb. 7, 2013, 3:40 p.m. Open
Is this meant to be e.getCause() instance of? I don't follow the layers of instanceof. Brock Noland Feb. 7, 2013, 3:40 p.m. Open
Any throwable not matching the above is turned into an EventDeliveryException including Error and RuntimeException. I don't think we should ... Brock Noland Feb. 7, 2013, 3:40 p.m. Open
Not a huge deal, but we do this "Exception follows" messages all over flume. I don't really see a purpose ... Brock Noland Feb. 7, 2013, 3:40 p.m. Open
Same exception stuff as above Brock Noland Feb. 7, 2013, 3:40 p.m. Open
remove sysout Brock Noland Feb. 7, 2013, 3:40 p.m. Open
Looks like there are more than two stati, if so I think we should include the status in the error ... Brock Noland Feb. 7, 2013, 3:40 p.m. Open
remove sysout Brock Noland Feb. 7, 2013, 3:40 p.m. Open
converting all throwables to FlumeException should be handled better Brock Noland Feb. 7, 2013, 3:40 p.m. Open
Yep, good catch! Hari Shreedharan Feb. 7, 2013, 5:43 p.m. Open
Agreed, will update. Hari Shreedharan Feb. 7, 2013, 5:43 p.m. Open
Sure. Hari Shreedharan Feb. 7, 2013, 5:43 p.m. Open
This is O(n) ... we could make this faster. Mike Percy Feb. 9, 2013, 12:57 a.m. Open
This is O(n) ... can we get it down to O(1) or O(log n)? Mike Percy Feb. 9, 2013, 12:57 a.m. Open
Would a small amount of synchronization and a BlockingQueue make this code a little simpler? Brock Noland Feb. 10, 2013, 9:11 p.m. Open
I feel like we could do this in a little simpler fashion with a Semaphore? Brock Noland Feb. 10, 2013, 9:11 p.m. Open
Review request changed
Updated (Feb. 12, 2013, 12:02 a.m.)
  • changed from pending to submitted