Skip to content

Adding flatbuffers and migrating flatcc to flatbuffers for plasma #325

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
Merged
merged 14 commits into from
Mar 1, 2017
Merged

Adding flatbuffers and migrating flatcc to flatbuffers for plasma #325

merged 14 commits into from
Mar 1, 2017

Conversation

atumanov
Copy link
Contributor

No description provided.

robertnishihara
robertnishihara reviewed Feb 27, 2017
View reviewed changes
build.sh Outdated
@@ -25,7 +25,7 @@ bash "$ROOT_DIR/src/numbuf/thirdparty/build_thirdparty.sh"

# Now build everything.
pushd "$ROOT_DIR/python/core"
cmake ../..
cmake -DCMAKE_BUILD_TYPE=Release ../..
Copy link
Collaborator

Choose a reason for hiding this comment

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

will this cause all of the DCHECK's to stop working?

Copy link
Contributor

Choose a reason for hiding this comment

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

No

src/common/CMakeLists.txt Outdated
@@ -34,7 +34,7 @@ target_link_libraries(common "${CMAKE_CURRENT_LIST_DIR}/thirdparty/hiredis/libhi

function(define_test test_name library)
add_executable(${test_name} test/${test_name}.c ${ARGN})
add_dependencies(${test_name} hiredis flatcc)
add_dependencies(${test_name} hiredis flatcc flatbuffers_ep)
target_link_libraries(${test_name} common ${FLATBUFFERS_STATIC_LIB} ${library})
target_compile_options(${test_name} PUBLIC "-DPLASMA_TEST -DPHOTON_TEST -DCOMMON_TEST -DRAY_COMMON_LOG_LEVEL=4 -DRAY_TIMEOUT=50")
endfunction()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing src/common/build/.gitkeep (not this file, but the next one in the PR) was unintentional, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same with src/global_scheduler/build/.gitkeep and src/photon/build/.gitkeep

Copy link
Contributor

Choose a reason for hiding this comment

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

These .gitkeeps are not used any more. Not sure it should be part of this PR but they should be removed...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see.

src/plasma/plasma_protocol.c Outdated
@@ -76,7 +76,10 @@ int finalize_buffer_and_send(flatcc_builder_t *B, int fd, int message_type) {
void *buff = flatcc_builder_finalize_buffer(B, &size);
int r = write_message(fd, message_type, size, buff);
free(buff);
flatcc_builder_reset(B);
if (!(message_type == MessageType_PlasmaCreateRequest || message_type == MessageType_PlasmaSealRequest)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still relevant?

src/plasma/plasma_protocol.cc Outdated
auto message = flatbuffers::GetRoot<PlasmaCreateRequest>(data);
*data_size = message->data_size();
*metadata_size = message->metadata_size();
CHECK(message->object_id()->size() == UNIQUE_ID_SIZE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably shouldn't use the UNIQUE_ID_SIZE macro, but instead use e.g., sizeof(*object_id).

src/plasma/plasma_protocol.cc Outdated
char **address,
int *port) {
DCHECK(data);
/* */
Copy link
Collaborator

Choose a reason for hiding this comment

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

/* */ is unused

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/116/
Test FAILed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/117/
Test PASSed.

@AmplabJenkins
Copy link

Merged build finished. Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/119/
Test PASSed.

@AmplabJenkins
Copy link

Merged build finished. Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/120/
Test PASSed.

@AmplabJenkins
Copy link

Merged build finished. Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/121/
Test PASSed.

@AmplabJenkins
Copy link

Merged build finished. Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/122/
Test PASSed.

@robertnishihara robertnishihara changed the title [WIP] adding flatbuffers and migrating flatcc to flatbuffers for plasma Adding flatbuffers and migrating flatcc to flatbuffers for plasma Feb 27, 2017
@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/125/
Test PASSed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/129/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/131/
Test FAILed.

@pcmoritz
Copy link
Contributor

Oh wow, thanks for catching that 🥇

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/135/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/136/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/138/
Test FAILed.

@AmplabJenkins
Copy link

Merged build finished. Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/141/
Test FAILed.

@robertnishihara robertnishihara merged commit b91d9cb into ray-project:master Mar 1, 2017
@robertnishihara robertnishihara deleted the flatbuffers-merge branch March 1, 2017 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcmoritz pcmoritz pcmoritz left review comments

@robertnishihara robertnishihara robertnishihara left review comments

@stephanie-wang stephanie-wang stephanie-wang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@atumanov @AmplabJenkins @pcmoritz @robertnishihara @stephanie-wang