You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Jonathan Heard (JIRA)" <ji...@apache.org> on 2015/10/27 12:47:27 UTC
[jira] [Comment Edited] (THRIFT-3397) Implement TProcessorFactory
in C# to enable per-client processors
[ 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)
Re: [jira] [Comment Edited] (THRIFT-3397) Implement TProcessorFactory
in C# to enable per-client processors
Posted by Randy Abernethy <ra...@gmail.com>.
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) <ji...@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)
>