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.

Sign up for GitHub

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

Jump to bottom

misc: Add toString() to OutputBuffer stats #11562

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

misc: Add toString() to OutputBuffer stats #11562

wants to merge 2 commits into from

Conversation

yingsu00
Copy link
Collaborator

@yingsu00 yingsu00 commented Nov 17, 2024 •
edited
Loading

Sample output:

[bufferedBytes: 0B, bufferedPages: 0, totalBytesSent: 6.69KB, totalRowsSent: 400, totalPagesSent: 4, averageBufferTimeMs: 0, numTopBuffers: 4] 
  D0: [finished: false, bytesBuffered: 0B, rowsBuffered: 0, pagesBuffered: 0, bytesSent: 1.67KB, rowsSent: 100, pagesSent:1]
  D1: [finished: false, bytesBuffered: 0B, rowsBuffered: 0, pagesBuffered: 0, bytesSent: 1.67KB, rowsSent: 100, pagesSent:1]
  D2: [finished: false, bytesBuffered: 0B, rowsBuffered: 0, pagesBuffered: 0, bytesSent: 1.67KB, rowsSent: 100, pagesSent:1]
  D3: [finished: false, bytesBuffered: 0B, rowsBuffered: 0, pagesBuffered: 0, bytesSent: 1.67KB, rowsSent: 100, pagesSent:1]

@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 17, 2024
@yingsu00 yingsu00 requested a review from xiaoxmeng November 17, 2024 04:38
@netlify Netlify
Copy link

netlify bot commented Nov 17, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit a005bbb
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/675b5e9d064f7800089bc3f2

xiaoxmeng
xiaoxmeng reviewed Nov 18, 2024
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@yingsu00 thanks for the improvement. Do you want to have a unit test for this? thanks!

velox/exec/OutputBuffer.h Outdated
return fmt::format(
"[{}, {}, {}, {}, {}, {}, {}]",
finished,
bytesBuffered,
Copy link
Contributor

Choose a reason for hiding this comment

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

use succinctBytes for stats in bytes

velox/exec/OutputBuffer.h Show resolved Hide resolved
velox/exec/OutputBuffer.h Outdated
}

return fmt::format(
"[ bufferedBytes: {}, bufferedPages: {}, "
Copy link
Contributor

Choose a reason for hiding this comment

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

[bufferedBytes

velox/exec/OutputBuffer.h Outdated
"[ bufferedBytes: {}, bufferedPages: {}, "
"totalBytesSent: {}, totalRowsSent: {}, totalPagesSent: {}, "
"averageBufferTimeMs: {}, numTopBuffers: {}, {}]",
bufferedBytes,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

velox/exec/OutputBuffer.h Outdated
std::string destinationBufferStats;
if (!buffersStats.empty()) {
for (auto& destinationBufferStat : buffersStats) {
destinationBufferStats += destinationBufferStat.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want one line per each destination buffer given there would be hundreds of them for better display? Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I have updated this PR to one line per destination buffer

@yingsu00
Copy link
Collaborator Author

@xiaoxmeng Thanks for reviewing! I haven't finished the test yet, but here's the output what it looks like:

[bufferedBytes: 0B, bufferedPages: 0, totalBytesSent: 6.69KB, totalRowsSent: 400, totalPagesSent: 4, averageBufferTimeMs: 0, numTopBuffers: 4] 
  D0: [finished: false, bytesBuffered: 0B, rowsBuffered: 0, pagesBuffered: 0, bytesSent: 1.67KB, rowsSent: 100, pagesSent:1]
  D1: [finished: false, bytesBuffered: 0B, rowsBuffered: 0, pagesBuffered: 0, bytesSent: 1.67KB, rowsSent: 100, pagesSent:1]
  D2: [finished: false, bytesBuffered: 0B, rowsBuffered: 0, pagesBuffered: 0, bytesSent: 1.67KB, rowsSent: 100, pagesSent:1]
  D3: [finished: false, bytesBuffered: 0B, rowsBuffered: 0, pagesBuffered: 0, bytesSent: 1.67KB, rowsSent: 100, pagesSent:1]

Let me know if it looks ok.

@yingsu00 yingsu00 force-pushed the OutputBufferToString branch from 5a4513c to 3df500d Compare November 20, 2024 00:33
@yingsu00 yingsu00 force-pushed the OutputBufferToString branch 2 times, most recently from d246b66 to 503e0fb Compare November 20, 2024 01:13
assignUser
assignUser requested changes Nov 20, 2024
View reviewed changes
velox/common/testutil/CMakeLists.txt Outdated Show resolved Hide resolved
@yingsu00 yingsu00 force-pushed the OutputBufferToString branch 5 times, most recently from 97f067b to e9b83f4 Compare November 26, 2024 01:12
@yingsu00
Copy link
Collaborator Author

@assignUser @pedroerp @xiaoxmeng The tests are green now, could you please review again? The current CMakelists.txt looks like this

velox_add_library(velox_test_util ScopedTestTime.cpp TestValue.cpp)
velox_link_libraries(velox_test_util PUBLIC velox_exception)

velox_add_library(velox_test_output_matcher OutputMatcher.cpp)
velox_link_libraries(velox_test_output_matcher PUBLIC Folly::folly GTest::gtest re2::re2)

Thank you!

assignUser
assignUser reviewed Nov 27, 2024
View reviewed changes
velox/common/testutil/CMakeLists.txt Outdated
Comment on lines 18 to 20
velox_add_library(velox_test_output_matcher OutputMatcher.cpp)
velox_link_libraries(velox_test_output_matcher PUBLIC Folly::folly GTest::gtest
re2::re2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like this it will still fail the build due to missing GTest when VELOX_BUILD_TESTING is not set. But because of the refactor you can just move it into the if and that should be fine (it's just used in tests right?)

@yingsu00 yingsu00 force-pushed the OutputBufferToString branch from e9b83f4 to a5e42e1 Compare November 28, 2024 00:40
assignUser
assignUser approved these changes Nov 28, 2024
View reviewed changes
Copy link
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

CMake looks good now, thanks!

@yingsu00 yingsu00 force-pushed the OutputBufferToString branch from a5e42e1 to ee05acf Compare December 3, 2024 22:13
@yingsu00
Copy link
Collaborator Author

yingsu00 commented Dec 4, 2024

@xiaoxmeng I've addressed all comments and added test. The tests all pass now. Do you want to review again? Many thanks!

xiaoxmeng
xiaoxmeng approved these changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@yingsu00 thanks for the update % nit.

velox/exec/OutputBuffer.h Outdated
std::string toString() const {
return fmt::format(
"[finished: {}, bytesBuffered: {}, rowsBuffered: {}, pagesBuffered: {}, bytesSent: {}, rowsSent: {}, pagesSent:{}]",
// "[{}, {}, {}, {}, {}, {}, {}]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove //?

@yingsu00 yingsu00 force-pushed the OutputBufferToString branch from ee05acf to a005bbb Compare December 12, 2024 22:07
@xiaoxmeng xiaoxmeng 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 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedroerp pedroerp pedroerp left review comments

@xiaoxmeng xiaoxmeng xiaoxmeng approved these changes

@assignUser assignUser assignUser approved these changes

@majetideepak majetideepak Awaiting requested review from majetideepak majetideepak is a code owner

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yingsu00 @pedroerp @assignUser @xiaoxmeng @facebook-github-bot