Skip to content

Change data ownership contract in call of ExchangeSink.add#13798

Merged
losipiuk merged 1 commit intotrinodb:masterfrom
losipiuk:lo/exchange-sink-data-owner-change
Aug 24, 2022
Merged

Change data ownership contract in call of ExchangeSink.add#13798
losipiuk merged 1 commit intotrinodb:masterfrom
losipiuk:lo/exchange-sink-data-owner-change

Conversation

@losipiuk
Copy link
Copy Markdown
Member

After the change the contract changes in a way that ownership of data
passed as an argument is passed from caller to callee.
Apart from usage in DeduplicatingDirectExchangeBuffer the Slices passed
as to ExchangeSink.add were single-use and not mutataded/reclaimed
afterwards.

The semantics change allows for simplere and more effective
implementations of ExchangeSinks as no extra data copying is enforced.

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Aug 23, 2022
@losipiuk losipiuk requested a review from arhimondr August 23, 2022 16:41
@losipiuk
Copy link
Copy Markdown
Member Author

@martint, @arhimondr do you think we need release notes for that? Technically it is SPI change yet I doubt there are any out of the tree implementations of this SPI part.

@arhimondr
Copy link
Copy Markdown
Contributor

LGTM % checkstyle failure

@findepi
Copy link
Copy Markdown
Member

findepi commented Aug 24, 2022

Technically it is SPI change yet I doubt there are any out of the tree implementations of this SPI part.

BTW you can mark the interface @Experimental.

@losipiuk
Copy link
Copy Markdown
Member Author

Technically it is SPI change yet I doubt there are any out of the tree implementations of this SPI part.

BTW you can mark the interface @Experimental.

I did not know that is the convention.
I will send separate PR with that. There is some SPI evolution expected in this region still.

After the change the contract changes in a way that ownership of data
passed as an argument is passed from caller to callee.
Apart from usage in DeduplicatingDirectExchangeBuffer the Slices passed
as to ExchangeSink.add were single-use and not mutataded/reclaimed
afterwards.

The semantics change allows for simplere and more effective
implementations of ExchangeSinks as no extra data copying is enforced.
@losipiuk losipiuk force-pushed the lo/exchange-sink-data-owner-change branch from 6ae2264 to bd84afe Compare August 24, 2022 08:33
@losipiuk losipiuk merged commit a977d88 into trinodb:master Aug 24, 2022
@github-actions github-actions bot added this to the 394 milestone Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants