Hi Johnathan, We try to avoid monster patches because they are hard to review. That said, if everything is focused on adding and supplying tests for a single issue, then one patch or pull request is probably appropriate.
Best, Randy On Tue, Oct 27, 2015 at 4:47 AM, Jonathan Heard (JIRA) <j...@apache.org> wrote: > > [ > https://issues.apache.org/jira/browse/THRIFT-3397?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14976263#comment-14976263 > ] > > Jonathan Heard edited comment on THRIFT-3397 at 10/27/15 11:47 AM: > ------------------------------------------------------------------- > > Please can I have some advice about how you like to do things in the > Thrift team... I'm close to submitting my pull request but would like to > check a few things. > > My local branch for this issue has the following commits: > 7bca7ff THRIFT-3397 Implemented \-\-server-type flag or C# TestServer in > line with test/README.md examples. Added '\-\-processor' flag to enable > selection of new TPrototypeProcessorFactory (--prototype OR > --processor=prototype) > 333b77d THRIFT-3397 Remove testStop() method. It's not in the > ThriftTest.thrift spec and when using TPrototypeFactory it becomes tricky > to allow a handler the ability to stop its own server. > a9dced0 THRIFT-3397 Set windows console height to maximum and enlarge > scrollback buffer > c52ced6 THRIFT-3397 Added clientID counter and logging delegate to > TestClient.cs, refactored Console.Writeline in tests to Invoke the delegate > with an implementation which automatically adds the clientID to the output. > 965bfc8 THRIFT-3397 TestClient was calling Open then Close on every new > Transport (i.e. Client thread '-t n') instance before passing it back to > the caller. Modified so it now does this only once > 95ee034 THRIFT-3397 Created TProcessorFactory interface, implemented at > TSingletonProcessorFactory and TPrototypeProcessorFactory and updated > TServer + implementations to use the singleton factory by default > > I have tried to ensure that each commit is limited to a particular change > to aide traceability, however it looks like most of the existing history > contains a single commit per JIRA ticket. > > What granularity is preferred for change control? e.g. Should I be > logging separate JIRA tickets for Library code vs Test code? > > This is my first contribution to the project so I want to ensure that I > get it right. > > > was (Author: jonathanh): > Please can I have some advice about how you like to do things in the > Thrift team... I'm close to submitting my pull request but would like to > check a few things. > > My local branch for this issue has the following commits: > 7bca7ff THRIFT-3397 Implemented --server-type flag or C# TestServer in > line with test/README.md examples. Added '--processor' flag to enable > selection of new TPrototypeProcessorFactory (--prototype OR > --processor=prototype) > 333b77d THRIFT-3397 Remove testStop() method. It's not in the > ThriftTest.thrift spec and when using TPrototypeFactory it becomes tricky > to allow a handler the ability to stop its own server. > a9dced0 THRIFT-3397 Set windows console height to maximum and enlarge > scrollback buffer > c52ced6 THRIFT-3397 Added clientID counter and logging delegate to > TestClient.cs, refactored Console.Writeline in tests to Invoke the delegate > with an implementation which automatically adds the clientID to the output. > 965bfc8 THRIFT-3397 TestClient was calling Open then Close on every new > Transport (i.e. Client thread '-t n') instance before passing it back to > the caller. Modified so it now does this only once > 95ee034 THRIFT-3397 Created TProcessorFactory interface, implemented at > TSingletonProcessorFactory and TPrototypeProcessorFactory and updated > TServer + implementations to use the singleton factory by default > > I have tried to ensure that each commit is limited to a particular change > to aide traceability, however it looks like most of the existing history > contains a single commit per JIRA ticket. > > What granularity is preferred for change control? e.g. Should I be > logging separate JIRA tickets for Library code vs Test code? > > This is my first contribution to the project so I want to ensure that I > get it right. > > > Implement TProcessorFactory in C# to enable per-client processors > > ----------------------------------------------------------------- > > > > Key: THRIFT-3397 > > URL: https://issues.apache.org/jira/browse/THRIFT-3397 > > Project: Thrift > > Issue Type: New Feature > > Components: C# - Library > > Affects Versions: 0.9.3 > > Reporter: Jonathan Heard > > Priority: Minor > > Labels: performance, thread-safety > > Original Estimate: 168h > > Remaining Estimate: 168h > > > > The current TServer implementations in C# take a single instance of a > TProcessor to be used for all client connections. This is not guaranteed to > be thread-safe and can become a significant bottleneck. > > I am implementing a TProcessorFactory interface for C# which is similar > to the ones already implemented in Java and C++. I'll generate a pull > request for review soon. > > The existing solutions implement TProcessorFactory as a class which > takes a TProcessor and just returns that instance to all clients. I'm > aiming to improve on that by creating a TProcessorFactory interface and > then implementing it in two core classes: > > 1) TSingletonProcessorFactory - this behaves the same as the Java > 'TProcessorFactory' class. > > 2) TPrototypeProcessorFactory<Processor,Handler> - this effectively > returns a 'new Processor(Handler)' giving every new client its own > processor. > > In order to avoid breaking the existing API (and in-keeping with the > Java implementation), I have changed TServer so that it uses a > TProcessorFactory instead of TProcessor and updated all relevant > constructors so they call the TSingletonProcessorFactory for constructors > using TProcessor. New constructors have been added to enable calling with > TProcessorFactory. This approach should avoid breaking the established API > and not break any existing code. > > I've also updated the TestServer.cs to support these changes. > > > > -- > This message was sent by Atlassian JIRA > (v6.3.4#6332) >
