Add streaming Writer implementation #490

dianale31 · 2021-06-08T20:22:00Z

This implementation extends the existing Binary writer struct to include the new streaming methods. Some refactoring is done in the main writer.go file to reduce common code.

Tests have also been added to test for correctness of the new streaming methods, but we'll need to write extensive tests for code generation, where we will actually assert that container types have the proper elements.

codecov · 2021-06-08T20:24:11Z

Codecov Report

Merging #490 (37ad6d1) into streamdev (fe65efe) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 37ad6d1 differs from pull request most recent head 228c133. Consider uploading reports for the commit 228c133 to get more accurate results

@@            Coverage Diff             @@
##           streamdev     #490   +/-   ##
==========================================
  Coverage      78.99%   78.99%           
==========================================
  Files            126      126           
  Lines          16067    16071    +4     
==========================================
+ Hits           12692    12696    +4     
  Misses          2065     2065           
  Partials        1310     1310

Impacted Files	Coverage Δ
idl/error.go	`100.00% <100.00%> (ø)`
idl/internal/parser.go	`100.00% <100.00%> (ø)`
idl/parser.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe65efe...228c133. Read the comment docs.

witriew

We'll probably be able to improve upon test coverage once both the stream.Writer and the stream.Reader land, then we can repeat tests similar to those done with "encode <-> decode".

protocol/binary/stream_writer.go

protocol/binary/writer.go

protocol/binary/stream_writer.go

protocol/binary_test.go

dianale31 · 2021-06-09T19:13:56Z

protocol/value_test.go

+	return testValue{val: val}
+}
+
+func (v testValue) streamEncodeStruct(sw *binary.StreamWriter) error {


I couldn't add these stream methods directly onto the wire.Value object because it would have caused an import cycle. Wrapped it here and made it explicit that this is only for testing since objects using the wire.Value intermediate representation should continue to use the binary.Encode and binary.Decode methods.

protocol/binary/stream_writer.go

protocol/binary_test.go

protocol/value_test.go

abhinav

This is a good first stab. I'd like to reduce the duplication in the implementation and the tests.
We should be able to reuse most of the existing test cases to exercise both code paths, especially if binary.Writer is implemented in terms of binary.StreamWriter.

protocol/binary/stream_writer.go

protocol/binary_test.go

protocol/value_test.go

protocol/binary/stream_writer.go

protocol/binary/writer.go

protocol/binary/stream_writer.go

protocol/binary_test.go

abhinav

Thanks for updating! This is generally looking great, except:

Looks like a couple commits from dev unintentionally ended up on this PR
There are a couple other spots in binary.Writer that it should use the exported StreamWriter methods rather than unexported
I left some optional minor stylistic comments for the tests

CHANGELOG.md

protocol/binary/writer.go

protocol/binary_test.go

abhinav

LGTM! Thanks for making the changes.

protocol/binary_test.go

r-hang · 2021-06-21T20:37:52Z

protocol/binary/stream_writer.go

+
+var streamWriterPool = sync.Pool{
+	New: func() interface{} {
+		writer := &StreamWriter{}


&StreamWriter{} can be returned.

protocol/binary/stream_writer.go

protocol/binary_test.go

r-hang · 2021-06-21T21:32:13Z

protocol/binary_test.go

+func TestStructBeginAndEndEncode(t *testing.T) {
+	var (
+		streamBuff bytes.Buffer
+		err        error


Is there a need for the the err variable? I believe we can just require.NoError(t, err)? This comment applies to similar pattern throughout.

# Commits The following comments are included in this release. Some of these cherry-picked and released in v1.28.0, but they appear again in the list above. - protocol: Add streaming interfaces (#485) - Move Stream-based interfaces into their own package - Make Streaming interfaces private to allow for safe experimentation (#488) - idl: Return structured ParseError from idl.Parse() (#492) - Add CHANGELOG entry for #492 (#494) - Support "<" in the templating language (#499) - idl: add a Position struct to wrap reported lines (#497) - Add streamwriter implementation (#490) - Add a "StreamReader" which implements "stream.Reader" - Use the "stream.Reader" in the "binary.Reader" - Add code generation for all wire types for stream encoding (#500) - Generate "Decode" for "enums" that will directly decode (#495) - Provide "decode" code generation for the streaming variants for all other types (#496) - idl: record document positions on constant nodes (#503) - ast: move idl.Position to the ast package (#504) - idl: replace internal.Position with ast.Position (#505) - Expose stream protocol method to close Writer (#506) - idl: add column numbers to parse error positions (#507) - idl: record full positions for constants (#508) - Mark assertParseCases() as a test helper (#509) - protocol/stream: Define enveloping interfaces (#511) - protocol/stream: Declare interface for encoding envelopes (#513) - binary/StreamWriter: Borrow => New; unexport Return (#515) - stream: add Close method, pool binary reader (#514) - binary/reader: Return to pool after ReadValue (#517) - binary/reader: Skip fixed width collections faster (#518) - binary/stream/reader: Fast-path offsetReader skips (#519) - binary: Move Responders and Protocol into package (#516) - benchmark: Refactor into a suite (#520) - Upgrade to Ragel version 6.10 (from 6.9) (#523) - Responder: Deduplicate interface (#524) - gen/quick_test: Add missing types (#525) - enum/json: Support rejecting unknown values (#502) - Back to development - Upgrade to golang.org/x/tools version 0.1.5 (#529) - ast: add column values to the AST nodes (#522) - stream: Implement Request and Response handling with Enveloping (#526) - offsetReader: Implement io.Seeker - binary/ReadRequest: Use io.Seeker if available - StreamReader: Use Seeker instead of offsetReader - protocol/stream: Unembed stream.Protocol from stream.RequestReader (#532) - thrifttest: Add mocks for streaming interfaces (#527) - streaming: Unembed iface.Private in streaming-based interfaces (#533) - Regenerate files for tests after merging `streamdev` - ast: formally declare CppInclude as a Node (#536) - ast: add Annotations(Node) []*Annotations (#537) - Preparing release v1.29.0 # API changes I ran apidiff on all packages in v1.28.0 and compared it with this release. Removing changes to gen/internal/tests, the result is: ``` --- go.uber.org/thriftrw/ast --- Compatible changes: - Annotation.Column: added - Annotations: added - BaseType.Column: added - Constant.Column: added - ConstantList.Column: added - ConstantMap.Column: added - ConstantMapItem.Column: added - ConstantReference.Column: added - CppInclude.Column: added - DefinitionInfo.Column: added - Enum.Column: added - EnumItem.Column: added - Field.Column: added - Function.Column: added - Include.Column: added - ListType.Column: added - MapType.Column: added - Namespace.Column: added - Position.Column: added - Position.String: added - Service.Column: added - ServiceReference.Column: added - SetType.Column: added - Struct.Column: added - TypeReference.Column: added - Typedef.Column: added --- go.uber.org/thriftrw/envelope/stream --- NEW PACKAGE --- go.uber.org/thriftrw/gen --- Compatible changes: - StreamGenerator: added --- go.uber.org/thriftrw/internal/envelope/exception --- Compatible changes: - (*ExceptionType).Decode: added - (*TApplicationException).Decode: added - (*TApplicationException).Encode: added - ExceptionType.Encode: added --- go.uber.org/thriftrw/plugin/api --- Compatible changes: - (*Argument).Decode: added - (*Argument).Encode: added - (*Feature).Decode: added - (*Function).Decode: added - (*Function).Encode: added - (*GenerateServiceRequest).Decode: added - (*GenerateServiceRequest).Encode: added - (*GenerateServiceResponse).Decode: added - (*GenerateServiceResponse).Encode: added - (*HandshakeRequest).Decode: added - (*HandshakeRequest).Encode: added - (*HandshakeResponse).Decode: added - (*HandshakeResponse).Encode: added - (*Module).Decode: added - (*Module).Encode: added - (*ModuleID).Decode: added - (*Plugin_Goodbye_Args).Decode: added - (*Plugin_Goodbye_Args).Encode: added - (*Plugin_Goodbye_Result).Decode: added - (*Plugin_Goodbye_Result).Encode: added - (*Plugin_Handshake_Args).Decode: added - (*Plugin_Handshake_Args).Encode: added - (*Plugin_Handshake_Result).Decode: added - (*Plugin_Handshake_Result).Encode: added - (*Service).Decode: added - (*Service).Encode: added - (*ServiceGenerator_Generate_Args).Decode: added - (*ServiceGenerator_Generate_Args).Encode: added - (*ServiceGenerator_Generate_Result).Decode: added - (*ServiceGenerator_Generate_Result).Encode: added - (*ServiceID).Decode: added - (*SimpleType).Decode: added - (*Type).Decode: added - (*Type).Encode: added - (*TypePair).Decode: added - (*TypePair).Encode: added - (*TypeReference).Decode: added - (*TypeReference).Encode: added - Feature.Encode: added - ModuleID.Encode: added - ServiceID.Encode: added - SimpleType.Encode: added --- go.uber.org/thriftrw/protocol --- Compatible changes: - BinaryStreamer: added --- go.uber.org/thriftrw/protocol/binary --- Compatible changes: - Default: added - EnvelopeV0Responder: added - EnvelopeV1Responder: added - NewStreamReader: added - NewStreamWriter: added - NoEnvelopeResponder: added - Protocol: added - Responder: added - StreamReader: added - StreamWriter: added --- go.uber.org/thriftrw/protocol/envelope --- NEW PACKAGE --- go.uber.org/thriftrw/protocol/stream --- NEW PACKAGE --- go.uber.org/thriftrw/thrifttest/streamtest --- NEW PACKAGE --- go.uber.org/thriftrw/version --- Incompatible changes: - Version: value changed from "1.28.0" to "1.29.0" ```

witriew reviewed Jun 9, 2021

View reviewed changes

dianale31 marked this pull request as draft June 9, 2021 16:24

dianale31 force-pushed the dianale/writer branch 2 times, most recently from f7b8357 to aa086f6 Compare June 9, 2021 19:10

dianale31 commented Jun 9, 2021

View reviewed changes

dianale31 force-pushed the dianale/writer branch from aa086f6 to c38fede Compare June 9, 2021 19:17

dianale31 requested a review from witriew June 9, 2021 19:19

dianale31 marked this pull request as ready for review June 9, 2021 19:20

dianale31 requested a review from abhinav June 9, 2021 19:20

witriew reviewed Jun 10, 2021

View reviewed changes

protocol/binary/stream_writer.go Outdated Show resolved Hide resolved

protocol/binary/stream_writer.go Outdated Show resolved Hide resolved

protocol/binary_test.go Outdated Show resolved Hide resolved

protocol/binary_test.go Outdated Show resolved Hide resolved

witriew reviewed Jun 10, 2021

View reviewed changes

protocol/binary_test.go Outdated Show resolved Hide resolved

protocol/value_test.go Outdated Show resolved Hide resolved

abhinav reviewed Jun 11, 2021

View reviewed changes

protocol/binary/stream_writer.go Show resolved Hide resolved

protocol/binary_test.go Outdated Show resolved Hide resolved

protocol/value_test.go Outdated Show resolved Hide resolved

dianale31 force-pushed the dianale/writer branch 3 times, most recently from 77caf89 to 2319785 Compare June 15, 2021 16:51

dianale31 requested review from abhinav and witriew June 15, 2021 16:54

dianale31 force-pushed the dianale/writer branch from 2319785 to 228c133 Compare June 16, 2021 15:55

witriew reviewed Jun 16, 2021

View reviewed changes

protocol/binary/stream_writer.go Outdated Show resolved Hide resolved

protocol/binary/writer.go Outdated Show resolved Hide resolved

protocol/binary/stream_writer.go Outdated Show resolved Hide resolved

protocol/binary_test.go Outdated Show resolved Hide resolved

witriew mentioned this pull request Jun 16, 2021

Add code generation for all wire types for stream encoding #498

Closed

abhinav reviewed Jun 17, 2021

View reviewed changes

dianale31 force-pushed the dianale/writer branch 2 times, most recently from 81de41f to 4a4fd56 Compare June 21, 2021 16:00

dianale31 requested review from witriew and abhinav June 21, 2021 16:02

abhinav approved these changes Jun 21, 2021

View reviewed changes

protocol/binary_test.go Outdated Show resolved Hide resolved

protocol/binary_test.go Outdated Show resolved Hide resolved

abhinav mentioned this pull request Jun 21, 2021

Add a "StreamReader" and plumb it for use in the standard "binary.Reader" #491

Merged

r-hang reviewed Jun 22, 2021

View reviewed changes

Add streamwriter implementation

3cffdbc

dianale31 force-pushed the dianale/writer branch from 4a4fd56 to 3cffdbc Compare June 22, 2021 01:14

dianale31 merged commit 5075744 into thriftrw:streamdev Jun 22, 2021

dianale31 added a commit that referenced this pull request Jun 22, 2021

Add streamwriter implementation (#490)

d9e695a

abhinav mentioned this pull request Aug 30, 2021

Release v1.29.0 #539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add streaming Writer implementation #490

Add streaming Writer implementation #490

dianale31 commented Jun 8, 2021

codecov bot commented Jun 8, 2021 •

edited

Loading

witriew left a comment

dianale31 Jun 9, 2021

abhinav left a comment

abhinav left a comment

abhinav left a comment

r-hang Jun 21, 2021

r-hang Jun 21, 2021

Add streaming Writer implementation #490

Add streaming Writer implementation #490

Conversation

dianale31 commented Jun 8, 2021

codecov bot commented Jun 8, 2021 • edited Loading

Codecov Report

witriew left a comment

Choose a reason for hiding this comment

dianale31 Jun 9, 2021

Choose a reason for hiding this comment

abhinav left a comment

Choose a reason for hiding this comment

abhinav left a comment

Choose a reason for hiding this comment

abhinav left a comment

Choose a reason for hiding this comment

r-hang Jun 21, 2021

Choose a reason for hiding this comment

r-hang Jun 21, 2021

Choose a reason for hiding this comment

codecov bot commented Jun 8, 2021 •

edited

Loading