Change Composite Writer to Sequential Writer #930
Conversation
Signed-off-by: Davit Yeghshatyan <[email protected]>
Signed-off-by: Davit Yeghshatyan <[email protected]>
Signed-off-by: Davit Yeghshatyan <[email protected]>
davit-y
requested review from
black-adder,
jpkrohling,
pavolloffay,
vprithvi and
yurishkuro
as code owners
Codecov Report
@@ Coverage Diff @@
## master #930 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 126 126
Lines 6074 6073 -1
=====================================
- Hits 6074 6073 -1
Continue to review full report at Codecov.
|
| ) | ||
|
|
||
| // CompositeWriter is a span Writer that tries to save spans into several underlying span Writers | ||
| type CompositeWriter struct { | ||
| // SequentialWriter is a span Writer that tries to save spans into several underlying span Writers |
There was a problem hiding this comment.
Expression error semantics here. End sentences with periods.
| @@ -40,16 +39,16 @@ func (n *noopWriteSpanStore) WriteSpan(span *model.Span) error { | |||
| } | |||
|
|
|||
| func TestCompositeWriteSpanStoreSuccess(t *testing.T) { | |||
There was a problem hiding this comment.
Make test names match the type name
| c := NewCompositeWriter(&errProneWriteSpanStore{}, &errProneWriteSpanStore{}) | ||
| assert.EqualError(t, c.WriteSpan(nil), fmt.Sprintf("[%s, %s]", errIWillAlwaysFail, errIWillAlwaysFail)) | ||
| c := NewSequentialWriter(&errProneWriteSpanStore{}, &errProneWriteSpanStore{}) | ||
| assert.EqualError(t, c.WriteSpan(nil), errIWillAlwaysFail.Error()) |
There was a problem hiding this comment.
I don't think you can correctly test this way because you can't tell the difference between which writer returns the error. I suggest to change noop writer to take an optional err parameter that it always returns, and do a data driven test where you pass different permutations of 3 noop writers with and without errors and check for the right error in the end.
What about the second of three writers fail? |
|
The behavioral change here is not properly reflected by the name Sequential, the old composite writer was also sequential. The real change is fail-fast on errors. You may want to add an options struct to the constructor with a Boolean field failFast. This way you'll preserve the old opportunistic behavior. Regarding retries, I don't think it's in scope of this composition, it's up to individual writers. |
Do we require this opportunistic behavior? I like the name |
|
Changing the CompositeWriter was found to be unnecessary to complete the Ingester. In addition, the use case of a SequentialWriter is not yet well defined |
Which problem is this PR solving?
Short description of the changes