streamingccl: fix nil linter check error and stream_ingestion_test error #79935
Conversation
|
This change is |
gh-casper
force-pushed
the
nilness-check
branch
from
a3ed3f1 to
090fe25
Compare
|
@rickystewart Thanks for finding this! |
| event = streamingccl.MakeKVEvent(*keyValCopy) | ||
| // 'keyValCopy' will only be set when it is a KV event. Copying the 'keyValCopy' again | ||
| // to prevent the event being modified by interceptor again. | ||
| event = streamingccl.MakeKVEvent(*copyKeyVal(keyValCopy)) |
There was a problem hiding this comment.
perhaps are apis are not good? I don't understand why we need a copy of a copy...
Previously, the stream ingestion test has no error since no duplicate event is generated. This PR fixes this and nil linter check error at the same time. Release note: None
gh-casper
force-pushed
the
nilness-check
branch
from
090fe25 to
3df8165
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @miretskiy and @samiskin)
pkg/ccl/streamingccl/streamclient/random_stream_client.go, line 412 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
perhaps are apis are not good? I don't understand why we need a copy of a copy...
The reason we want to make a copy at the first place is that the generated KV event might get modified downstream. The flow looks like this: generate a event, make a copy for this event, event goes to streamclient's channel and gets modified, use the copy the create a event which will passed into interceptor which may modify the event. In the last step, if we use MakeKVEvent to use copy's value to create a event, then the copy will be modified as well. This copy should not be modified because it will be used when we generate the next duplicate event, so we have to create another copy.
Code quote:
streamingccl.MakeKVEvent
|
bors r+ |
|
Build succeeded: |
Previously, the stream ingestion test has no error since no
duplicate event is generated. This PR fixes this and nil linter
check error at the same time.
Release note: None
Closes: #79926
Related to #79929 which
disables random_stream_client nil linter check. Will re-enable it after it's
merged.