Skip to content

Conversation

@xiaoxmeng
Copy link
Contributor

No description provided.

@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 Mar 19, 2024
@netlify
Copy link

netlify bot commented Mar 19, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 66b9db8
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65fa03dcf53a650008aab169

@xiaoxmeng xiaoxmeng requested review from Yuhta and mbasmanova March 19, 2024 19:02
@xiaoxmeng xiaoxmeng marked this pull request as ready for review March 19, 2024 19:02
@facebook-github-bot
Copy link
Contributor

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


// Process-wide IO wait time.
static std::atomic<uint64_t> ioWaitNanos_;
inline static std::atomic<uint64_t> ioWaitNanos_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this into a global variable in cpp file only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a static method that report this which is now only used by test but could be used by production later so keep it here.

Copy link
Contributor

@Yuhta Yuhta Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the inline here create multiple instances and the value is no longer process-wide?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is static so it should be singleton, and used it in many places.

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@kevinwilfong kevinwilfong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

preload(connectorSplit);
} else if (
readySplitIndex == -1 && connectorSplit->dataSource->hasValue()) {
(readySplitIndex == -1) && (connectorSplit->dataSource->hasValue())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: are the additional parentheses necessary? IMO they make it less readable (but that's just an opinion)

@facebook-github-bot
Copy link
Contributor

@xiaoxmeng merged this pull request in 5e07790.

@xiaoxmeng xiaoxmeng deleted the scan branch March 20, 2024 03:57
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
Summary: Pull Request resolved: facebookincubator#9156

Reviewed By: kevinwilfong

Differential Revision: D55086413

Pulled By: xiaoxmeng

fbshipit-source-id: 60c557f7635e7eb498e17e46f1a15534d862b8bf
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. Merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants