> On 2012-03-24 00:47:27, Mike Percy wrote: > > Hi, thanks for doing this work. > > > > I have the following suggestions on the config format: > > > > The properties format is not extensible. Right now it looks like this: > > hosts = host1 host2 > > host1 = hostname1:port1 > > host2 = hostname2:port2 > > > > How about something like: > > hosts = host1 host2 > > host.host1.endpoint = hostname1:port1 > > host.host2.endpoint = hostname2:port2 > > > > In this way, we namespace the host specifications, and also have the option > > of adding stuff like groups or priorities to them later if we want. > > > > Also, I have reservations about using the enum and reflection. My thoughts > > regarding that are below.
I don't think we should make the properties more and more complex. I already have reservations about taking in a properties file. Namespacing should not be required if this is going to be kept simple. I know the client might have other properties data(if it is being loaded from a file, in that case namespacing it as just as "hosts" also can cause issues - we will need to make it something like flume.hosts or even org.apache.flume.hosts at which point we are overthinking it). Either way I would assume that the client code would be using some data of his own and creating this properties object and passing it to our code, rather than passing the config file entirely to our code anyway - open source aside, passing your config, (not pertinent to that component) is a bad idea. I think this would also be extensible: host1 = hostname1:port1 host2 = hostname2:port2 If we add groups we can do something like: host1.group = <blah> or group1.hosts = host1, host2 same applies for priorities. I think it is good to make the properties extensible, but making it more and more complex, expecting to add more features in the future is not something I want to do right now. I'd rather keep it simple, than make it complex when most of the features like groups and priorities(which also can be added as shown above) are not likely to be used very frequently. For now, I think this should be kept simple. - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4380/#review6312 ----------------------------------------------------------- On 2012-03-22 03:43:56, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4380/ > ----------------------------------------------------------- > > (Updated 2012-03-22 03:43:56) > > > Review request for Flume. > > > Summary > ------- > > Submitting an initial cut of FailoverRpcClient that uses the NettyRpcClient > under the hood. In this version, host selection is not exactly the best, > please make suggestions on how to improve it. As of now, the first version > will not have a backoff mechanism to not retry a host for a fixed time etc(as > discussed in the jira). I will add unit tests soon. > > Note that the actual "connect" call to a host is hidden from the > FailoverClient (by the Netty client or any other implementation, which we may > choose to use later). Since this connect call is hidden, failure to create a > client(the build function throwing an exception) is not being considered a > failure. Only a failure to append is considered a failure, and counted > towards the maximum number of tries. In other words, as far as the > FailoverClient(for that matter, any implementation of RpcClient interface) > would consider an append failure as failure, not a failure to a build() call > - if we want to make sure that a connect failure also is counted, we should > move the connect call to the append function and keep track of the connection > state internally, and not expect any code depending on an implementation of > RpcClient(including other clients which depend on pre-existing clients) to > know that a build call also creates a connection - this is exactly like a > socket implementation, creating a new socket does not initialize a > connection, it is done explicitly. > > > This addresses bug FLUME-962. > https://issues.apache.org/jira/browse/FLUME-962 > > > Diffs > ----- > > flume-ng-sdk/src/main/java/org/apache/flume/api/ClientType.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java > 965b2ff > flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java > 351b5b1 > flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 93bfee9 > flume-ng-sdk/src/test/java/org/apache/flume/api/TestFailoverRpcClient.java > PRE-CREATION > flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java > a33e9c8 > flume-ng-sdk/src/test/java/org/apache/flume/api/TestRpcClientFactory.java > 0c94231 > > Diff: https://reviews.apache.org/r/4380/diff > > > Testing > ------- > > Unit tests added for the new functionality > > > Thanks, > > Hari > >
