-
Notifications
You must be signed in to change notification settings - Fork 201
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
base: single-machine
Are you sure you want to change the base?
Conversation
Test FAILed. |
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'}"; |
There was a problem hiding this comment.
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.
libconfluo/confluo/atomic_multilog.h
Outdated
* @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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size_t append_json(std::string json_data); | |
size_t append_json(const std::string &json_data); |
libconfluo/confluo/schema/schema.h
Outdated
* | ||
* @return A pointer to the record data | ||
*/ | ||
void *json_string_to_data(const std::string json_record) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void *json_string_to_data(const std::string json_record) const; | |
void *json_string_to_data(const std::string &json_record) const; |
libconfluo/confluo/schema/schema.h
Outdated
* | ||
* @param record The records used for conversion | ||
* | ||
* @return A pointer to the record data |
There was a problem hiding this comment.
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..
libconfluo/src/atomic_multilog.cc
Outdated
@@ -228,6 +228,13 @@ size_t atomic_multilog::append(void *data) { | |||
return offset; | |||
} | |||
|
|||
size_t atomic_multilog::append_json(std::string json_record) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size_t atomic_multilog::append_json(std::string json_record) { | |
size_t atomic_multilog::append_json(const std::string &json_record) { |
libconfluo/src/schema/schema.cc
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
There was a problem hiding this comment.
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.
librpc/src/rpc_server.cc
Outdated
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 |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Test PASSed. |
/** | ||
* A json cursor that make records into a json formatted string | ||
*/ | ||
class aggregated_json_cursor : public json_cursor { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
fc7ecd1
to
0cce8a8
Compare
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
No description provided.