You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/10/14 18:22:30 UTC
[GitHub] [thrift] fishy opened a new pull request #2261: Add Dump protocol to go's TDebugProtocol
fishy opened a new pull request #2261:
URL: https://github.com/apache/thrift/pull/2261
This also comes from the discussion of
https://github.com/apache/thrift/pull/1992#issuecomment-705903922.
I think TDebugProtocol is a better fit for this feature than creating a
new TProtocol implementation.
The Dump field is not added to TDebugProtocolFactory because I don't
think it makes sense from the factory setup. In vast majority cases
users would need direct access to the underlying TMemoryBuffer to make
it useful, which is easier this way than an additional TTransportFactory
plus TProtocolFactory to make TDebugProtocolFactory way too complex.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2261: Add Dump protocol to go's TDebugProtocol
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2261:
URL: https://github.com/apache/thrift/pull/2261#discussion_r507890965
##########
File path: lib/go/thrift/debug_protocol.go
##########
@@ -25,9 +25,27 @@ import (
)
type TDebugProtocol struct {
- Delegate TProtocol
- LogPrefix string
+ // Required. The actual TProtocol to do the read/write.
+ Delegate TProtocol
+
+ // Optional. The logger and prefix to log all the args/return values
+ // from Delegate TProtocol calls.
+ //
+ // If Logger is nil, StdLogger using stdlib log package with os.Stderr
+ // will be used. If disable logging is desired, set Logger to NopLogger
+ // explicitly instead of leaving it as nil/unset.
Logger Logger
+ LogPrefix string
+
+ // Optional. An TProtocol to dump everything read/wrote from Delegate.
+ //
+ // A typical use case of this is to use TSimpleJSONProtocol wrapping
+ // TMemoryBuffer in a middleware to json logging requests/responses.
+ //
+ // This feature is not available from TDebugProtocolFactory. In order to
+ // use it you have to construct TDebugProtocol directly, or set Dump
+ // field after getting a TDebugProtocol from the factory.
+ Dump TProtocol
Review comment:
Those names don't sound right to me either 😕 What do you think about "WriteTo", "Writer", "DuplicateTo"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on a change in pull request #2261: Add Dump protocol to go's TDebugProtocol
Posted by GitBox <gi...@apache.org>.
dcelasun commented on a change in pull request #2261:
URL: https://github.com/apache/thrift/pull/2261#discussion_r507893929
##########
File path: lib/go/thrift/debug_protocol.go
##########
@@ -25,9 +25,27 @@ import (
)
type TDebugProtocol struct {
- Delegate TProtocol
- LogPrefix string
+ // Required. The actual TProtocol to do the read/write.
+ Delegate TProtocol
+
+ // Optional. The logger and prefix to log all the args/return values
+ // from Delegate TProtocol calls.
+ //
+ // If Logger is nil, StdLogger using stdlib log package with os.Stderr
+ // will be used. If disable logging is desired, set Logger to NopLogger
+ // explicitly instead of leaving it as nil/unset.
Logger Logger
+ LogPrefix string
+
+ // Optional. An TProtocol to dump everything read/wrote from Delegate.
+ //
+ // A typical use case of this is to use TSimpleJSONProtocol wrapping
+ // TMemoryBuffer in a middleware to json logging requests/responses.
+ //
+ // This feature is not available from TDebugProtocolFactory. In order to
+ // use it you have to construct TDebugProtocol directly, or set Dump
+ // field after getting a TDebugProtocol from the factory.
+ Dump TProtocol
Review comment:
Of those, DuplicateTo sounds best :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on a change in pull request #2261: Add Dump protocol to go's TDebugProtocol
Posted by GitBox <gi...@apache.org>.
dcelasun commented on a change in pull request #2261:
URL: https://github.com/apache/thrift/pull/2261#discussion_r507894357
##########
File path: lib/go/thrift/debug_protocol.go
##########
@@ -25,9 +25,27 @@ import (
)
type TDebugProtocol struct {
- Delegate TProtocol
- LogPrefix string
+ // Required. The actual TProtocol to do the read/write.
+ Delegate TProtocol
+
+ // Optional. The logger and prefix to log all the args/return values
+ // from Delegate TProtocol calls.
+ //
+ // If Logger is nil, StdLogger using stdlib log package with os.Stderr
+ // will be used. If disable logging is desired, set Logger to NopLogger
+ // explicitly instead of leaving it as nil/unset.
Logger Logger
+ LogPrefix string
+
+ // Optional. An TProtocol to dump everything read/wrote from Delegate.
+ //
+ // A typical use case of this is to use TSimpleJSONProtocol wrapping
+ // TMemoryBuffer in a middleware to json logging requests/responses.
+ //
+ // This feature is not available from TDebugProtocolFactory. In order to
+ // use it you have to construct TDebugProtocol directly, or set Dump
+ // field after getting a TDebugProtocol from the factory.
+ Dump TProtocol
Review comment:
Of those, DuplicateTo sounds best :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2261: Add DuplicateTo protocol to go's TDebugProtocol
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2261:
URL: https://github.com/apache/thrift/pull/2261#discussion_r507904838
##########
File path: lib/go/thrift/debug_protocol.go
##########
@@ -25,9 +25,27 @@ import (
)
type TDebugProtocol struct {
- Delegate TProtocol
- LogPrefix string
+ // Required. The actual TProtocol to do the read/write.
+ Delegate TProtocol
+
+ // Optional. The logger and prefix to log all the args/return values
+ // from Delegate TProtocol calls.
+ //
+ // If Logger is nil, StdLogger using stdlib log package with os.Stderr
+ // will be used. If disable logging is desired, set Logger to NopLogger
+ // explicitly instead of leaving it as nil/unset.
Logger Logger
+ LogPrefix string
+
+ // Optional. An TProtocol to dump everything read/wrote from Delegate.
+ //
+ // A typical use case of this is to use TSimpleJSONProtocol wrapping
+ // TMemoryBuffer in a middleware to json logging requests/responses.
+ //
+ // This feature is not available from TDebugProtocolFactory. In order to
+ // use it you have to construct TDebugProtocol directly, or set Dump
+ // field after getting a TDebugProtocol from the factory.
+ Dump TProtocol
Review comment:
OK renamed to `DuplicateTo`. commit message also updated accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2261: Add Dump protocol to go's TDebugProtocol
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2261:
URL: https://github.com/apache/thrift/pull/2261#discussion_r504882732
##########
File path: lib/go/thrift/debug_protocol.go
##########
@@ -25,9 +25,27 @@ import (
)
type TDebugProtocol struct {
- Delegate TProtocol
- LogPrefix string
+ // Required. The actual TProtocol to do the read/write.
+ Delegate TProtocol
+
+ // Optional. The logger and prefix to log all the args/return values
+ // from Delegate TProtocol calls.
+ //
+ // If Logger is nil, StdLogger using stdlib log package with os.Stderr
+ // will be used. If disable logging is desired, set Logger to NopLogger
+ // explicitly instead of leaving it as nil/unset.
Logger Logger
+ LogPrefix string
+
+ // Optional. An TProtocol to dump everything read/wrote from Delegate.
+ //
+ // A typical use case of this is to use TSimpleJSONProtocol wrapping
+ // TMemoryBuffer in a middleware to json logging requests/responses.
+ //
+ // This feature is not available from TDebugProtocolFactory. In order to
+ // use it you have to construct TDebugProtocol directly, or set Dump
+ // field after getting a TDebugProtocol from the factory.
+ Dump TProtocol
Review comment:
I guess "Dump" is not the best name, and I'm open to suggestions :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2261: Add Dump protocol to go's TDebugProtocol
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2261:
URL: https://github.com/apache/thrift/pull/2261#issuecomment-708579100
cc: @voodoo-dn
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on a change in pull request #2261: Add Dump protocol to go's TDebugProtocol
Posted by GitBox <gi...@apache.org>.
dcelasun commented on a change in pull request #2261:
URL: https://github.com/apache/thrift/pull/2261#discussion_r507643633
##########
File path: lib/go/thrift/debug_protocol.go
##########
@@ -25,9 +25,27 @@ import (
)
type TDebugProtocol struct {
- Delegate TProtocol
- LogPrefix string
+ // Required. The actual TProtocol to do the read/write.
+ Delegate TProtocol
+
+ // Optional. The logger and prefix to log all the args/return values
+ // from Delegate TProtocol calls.
+ //
+ // If Logger is nil, StdLogger using stdlib log package with os.Stderr
+ // will be used. If disable logging is desired, set Logger to NopLogger
+ // explicitly instead of leaving it as nil/unset.
Logger Logger
+ LogPrefix string
+
+ // Optional. An TProtocol to dump everything read/wrote from Delegate.
+ //
+ // A typical use case of this is to use TSimpleJSONProtocol wrapping
+ // TMemoryBuffer in a middleware to json logging requests/responses.
+ //
+ // This feature is not available from TDebugProtocolFactory. In order to
+ // use it you have to construct TDebugProtocol directly, or set Dump
+ // field after getting a TDebugProtocol from the factory.
+ Dump TProtocol
Review comment:
Hmm, "Export"? "Intercept"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy merged pull request #2261: Add DuplicateTo protocol to go's TDebugProtocol
Posted by GitBox <gi...@apache.org>.
fishy merged pull request #2261:
URL: https://github.com/apache/thrift/pull/2261
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org