Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(exchange): Rename taskId to remoteTaskId #11424

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lingbin
Copy link
Contributor

@lingbin lingbin commented Nov 4, 2024

Currently 'taskId' in an Exchange related class has two meanings, for
example ExchangeClient::taskId_ means local task (consumer),
ExchangeSource::taskId_ indicates a remote task (producer). This can
cause a bit of confusion.

And, in some places current codebase already uses 'remoteTaskId, such as
ExchangeClient::remoteTaskIds_. This patch renames the remaining
'task ID's representing the remote to 'remoteTaskId' to make it more
intuitive.

No functional changes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 4, 2024
Copy link

netlify bot commented Nov 4, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 429a844
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67ad705ea8c8f40008599825

@lingbin lingbin force-pushed the refactor-exchange-taskId branch 2 times, most recently from bc69480 to 9f312a8 Compare November 11, 2024 02:38
@lingbin
Copy link
Contributor Author

lingbin commented Nov 18, 2024

@Yuhta Could you please take a look, thanks.

@lingbin lingbin changed the title Rename taskId to remoteTaskId for readability in Exchange refactor(Exchange): Rename taskId to remoteTaskId for readability Nov 18, 2024
@lingbin lingbin changed the title refactor(Exchange): Rename taskId to remoteTaskId for readability refactor(exchange): Rename taskId to remoteTaskId for readability Nov 18, 2024
@lingbin lingbin changed the title refactor(exchange): Rename taskId to remoteTaskId for readability refactor(exchange): Rename taskId to remoteTaskId Nov 18, 2024
@lingbin lingbin force-pushed the refactor-exchange-taskId branch 4 times, most recently from f89262a to be29d58 Compare December 23, 2024 13:11
@Yuhta Yuhta added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Dec 27, 2024
@facebook-github-bot
Copy link
Contributor

@kevinwilfong has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@kevinwilfong
Copy link
Contributor

@lingbin it looks like Presto is currently using the field taskId_ here https://github.com/prestodb/presto/blob/942cbbc7fb79e0ad04a1a164a6960eae91219a02/presto-native-execution/presto_cpp/main/PrestoExchangeSource.h#L146

Is there a way to do this in a backwards compatible way so that Presto isn't broken? E.g. since taskId_ is constant you could do this in 3 steps:

  • land a change in Velox to add a remoteTaskId_ member alongside it initialized to the same value (ugly but temporary)
  • land a change in Presto to start using remoteTaskId_ instead
  • land a change in Velox to remove taskId_

@kgpai
Copy link
Contributor

kgpai commented Feb 12, 2025

@lingbin Gentle reminder.

lingbin and others added 2 commits February 13, 2025 11:46
Currently 'taskId' in an Exchange related class has two meanings, for
example `ExchangeClient::taskId_` means local task (consumer),
`ExchangeSource::taskId_` indicates a remote task (producer). This can
cause a bit of confusion.

And, in some places current codebase already use 'remoteTaskId, such as
`ExchangeClient::remoteTaskIds_`. This patch renames the remaining
'task ID's representing the remote to 'remoteTaskId' to make it more
intuitive.

No functional changes.
@lingbin lingbin force-pushed the refactor-exchange-taskId branch from be29d58 to 429a844 Compare February 13, 2025 04:09
@lingbin
Copy link
Contributor Author

lingbin commented Feb 13, 2025

Is there a way to do this in a backwards compatible way so that Presto isn't broken? E.g. since taskId_ is constant you could do this in 3 steps:

  • land a change in Velox to add a remoteTaskId_ member alongside it initialized to the same value (ugly but temporary)
  • land a change in Presto to start using remoteTaskId_ instead
  • land a change in Velox to remove taskId_

@kevinwilfong Thanks very much. I have changed the code according to your suggestions. And I will continue to follow up and eventually remove the temporary 'taskId_'.

@lingbin
Copy link
Contributor Author

lingbin commented Feb 13, 2025

@kevinwilfong Could you please help take a look again? cc @kgpai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants