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

JSON interface for RPC Client [Issue #14] #109

Open
wants to merge 6 commits into
base: single-machine
Choose a base branch
from

Conversation

attaluris
Copy link
Collaborator

No description provided.

@AmplabJenkins
Copy link
Collaborator

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

@AmplabJenkins
Copy link
Collaborator

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

@@ -158,14 +185,16 @@ TEST_F(AtomicMultilogTest, AppendAndGetDurableRelaxedTest) {
TEST_F(AtomicMultilogTest, AppendAndGetJSONRecordTest1) {
atomic_multilog mlog("my_table", s, "/tmp", storage::IN_MEMORY, archival_mode::OFF, MGMT_POOL);

std::string rec1 = "{'a':'false', 'b':'0', 'c':'0', 'd':'0', 'e':'0', 'f':'0.000000', 'g':'0.010000', 'h':'abc'}";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also test if this syntax works -- this would be the main use-case, I don't expect users to convert it to JSON and then back to string as the test now does.

* @param json_data The json-formatted data to be stored
* @return The offset of where the data is located
*/
size_t append_json(std::string json_data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
size_t append_json(std::string json_data);
size_t append_json(const std::string &json_data);

*
* @return A pointer to the record data
*/
void *json_string_to_data(const std::string json_record) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
void *json_string_to_data(const std::string json_record) const;
void *json_string_to_data(const std::string &json_record) const;

*
* @param record The records used for conversion
*
* @return A pointer to the record data
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not return anything..

@@ -228,6 +228,13 @@ size_t atomic_multilog::append(void *data) {
return offset;
}

size_t atomic_multilog::append_json(std::string json_record) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
size_t atomic_multilog::append_json(std::string json_record) {
size_t atomic_multilog::append_json(const std::string &json_record) {

@@ -94,6 +94,50 @@ std::string schema_t::to_string() const {
return str;
}

void *schema_t::json_string_to_data(const std::string json_record) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
void *schema_t::json_string_to_data(const std::string json_record) const {
void *schema_t::json_string_to_data(const std::string &json_record) const {


std::stringstream ss;
pt::write_json(ss, root);
std::string ret = ss.str();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe try printing this out and use the printed string as your test input? Tests are also useful as sample usage for users.

void rpc_service_handler::read_json(std::string &_return, int64_t id, const int64_t offset, const int64_t nrecords) {
atomic_multilog *mlog = store_->get_atomic_multilog(id);
_return = mlog->read_json((uint64_t) offset);
// TODO: put in functionality for nrecords to be read
Copy link
Contributor

Choose a reason for hiding this comment

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

Throw unsupported exception instead of incorrect functionality...


std::stringstream ss;
pt::write_json(ss, root);
std::string ret = ss.str();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here -- need to figure out what correct JSON string format is. We need to document it somewhere too.


std::stringstream ss;
pt::write_json(ss, root);
std::string ret = ss.str();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before.

@@ -0,0 +1,299 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should not be checked in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

running genall.sh resulted in this; were there supposed to be no changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I know it generates this file, but we don't want to check it in. Ideally the genall.sh file should delete it explicitly -- I can add that change, but can you add a commit to remove the file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good; are there any other files that shouldn't be checked in?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, that's the only one.

@AmplabJenkins
Copy link
Collaborator

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

/**
* A json cursor that make records into a json formatted string
*/
class aggregated_json_cursor : public json_cursor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need any aggregation

* @param cexpr The filter expression
* @param batch_size The number of records in the batch
*/
aggregated_json_cursor(std::unique_ptr<offset_cursor> o_cursor,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should take std::unique_ptr<record_cursor> as first argument, const schema_t *as second argument (for converting record to json) and size_t batch_size as the third and last argument.

size_t i = 0;
for (; i < current_batch_.size() && o_cursor_->has_more();
++i, o_cursor_->advance()) {
uint64_t o = o_cursor_->get();
Copy link
Contributor

Choose a reason for hiding this comment

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

None of this logic will be needed; only need to convert each record in record_cursor to JSON.

@AmplabJenkins
Copy link
Collaborator

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

@AmplabJenkins
Copy link
Collaborator

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

@AmplabJenkins
Copy link
Collaborator

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

@AmplabJenkins
Copy link
Collaborator

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

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.

None yet

3 participants