Skip to content

[CELEBORN-1754][CIP-14] Add exceptions and checking utils to cppClient#2966

Closed
HolyLow wants to merge 4 commits intoapache:mainfrom
HolyLow:issue/celeborn-1754-add-exceptions-utils-to-cppClient
Closed

[CELEBORN-1754][CIP-14] Add exceptions and checking utils to cppClient#2966
HolyLow wants to merge 4 commits intoapache:mainfrom
HolyLow:issue/celeborn-1754-add-exceptions-utils-to-cppClient

Conversation

@HolyLow
Copy link
Copy Markdown
Contributor

@HolyLow HolyLow commented Dec 1, 2024

What changes were proposed in this pull request?

This PR adds exceptions and checking utils code to CppClient.
Besides, the ctest framework is added to CppClient for UTs.

Why are the changes needed?

To provide exception utils and UT frmework to CppClient.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Compilation and UTs.

@HolyLow
Copy link
Copy Markdown
Contributor Author

HolyLow commented Dec 1, 2024

@FMX Could you kindly help review this PR? Thanks a lot.
Besides the exceptions and checking logic, the UT framework is supported now, so the uts are added.

Also note that the prebuild dev image is updated to holylow/celeborn-cpp-dev:0.2 to add gtest module dependency.

Any suggestion is welcome.

@FMX
Copy link
Copy Markdown
Contributor

FMX commented Dec 2, 2024

Copy that. I'll review this PR within this week.

@FMX
Copy link
Copy Markdown
Contributor

FMX commented Dec 4, 2024

@HolyLow I found out that I can run the UTs successfully.

If I run the UTs in Clion, I got following error.

1: Test command: /tmp/cpp/cmake-build-debug/celeborn/utils/tests/celeborn_utils_test
1: Working Directory: /tmp/cpp/cmake-build-debug/celeborn/utils/tests
1: Test timeout computed to be: 1500
Exception: Illegal

If I run the UTs in Docker container, I got following errors.

root@82f7ea7a535c:/celeborn/cpp/build# ctest
Test project /celeborn/cpp/build
    Start 1: celeborn_utils_test
1/1 Test #1: celeborn_utils_test ..............***Exception: Illegal  0.02 sec
0% tests passed, 1 tests failed out of 1


Total Test time (real) =   0.04 sec

The following tests FAILED:
	  1 - celeborn_utils_test (ILLEGAL)
Errors while running CTest
Output from these tests are in: /celeborn/cpp/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
root@82f7ea7a535c:/celeborn/cpp/build# cat /celeborn/cpp/build/Testing/Temporary/LastTest.log
Start testing: Dec 04 02:55 UTC
----------------------------------------------------------
1/1 Testing: celeborn_utils_test
1/1 Test: celeborn_utils_test
Command: "/celeborn/cpp/build/celeborn/utils/tests/celeborn_utils_test"
Directory: /celeborn/cpp/build/celeborn/utils/tests
"celeborn_utils_test" start time: Dec 04 02:55 UTC
Output:
----------------------------------------------------------
<end of output>
Test time =   0.02 sec
----------------------------------------------------------
Test Failed.
"celeborn_utils_test" end time: Dec 04 02:55 UTC
"celeborn_utils_test" time elapsed: 00:00:00
----------------------------------------------------------

End testing: Dec 04 02:55 UTC

@FMX
Copy link
Copy Markdown
Contributor

FMX commented Dec 4, 2024

Looks like there is something wrong with the UT, I'll merge this PR after it is fixed.

@FMX
Copy link
Copy Markdown
Contributor

FMX commented Dec 4, 2024

After some investigation, I found the errors that occurred were related to the difference between the arm64 and the x86 platforms. I'll merge this PR. Docker saved the day.

Copy link
Copy Markdown
Contributor

@FMX FMX 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. Merged into main(v0.6.0).

@FMX FMX closed this in b2b9a0a Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants