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

General modification for storage server library #59

Merged

Conversation

charles-typ
Copy link
Collaborator

@charles-typ charles-typ commented Jun 9, 2019

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/jiffy-prb/32/
Test FAILed.

if (impl_ == nullptr) {
throw std::invalid_argument("No such type " + type);
}
}

void block::destroy() {
LOG(log_level::info) << "Destroying partition " << impl_->name() << " on block " << id_;
//LOG(log_level::info) << "Destroying partition " << impl_->name() << " on block " << id_;
Copy link
Member

Choose a reason for hiding this comment

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

Put back the log?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

libjiffy/src/jiffy/storage/chain_module.h Outdated Show resolved Hide resolved
std::vector<command> OPS,
int timeout_ms)
: fs_(std::move(fs)), path_(path), status_(status), timeout_ms_(timeout_ms) {
(void) OPS;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

const std::string &auto_scaling_host,
int auto_scaling_port)
: chain_module(manager, name, metadata, {}) {
(void) directory_host;
Copy link
Member

Choose a reason for hiding this comment

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

I am guessing you are doing this to suppress the unused warning. Instead of (void) varname;, you can just avoid naming the variables, e.g.,

default_partition::default_partition(block_memory_manager *manager,
                                      const std::string &name,
                                      const std::string &metadata,
                                      const utils::property_map &,
                                      const std::string &,
                                      int,
                                      const std::string &,
                                      int)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly! Thanks for the suggestion.
Fixed

@@ -16,7 +16,7 @@ void storage_manager::create_partition(const std::string &block_id,
const std::map<std::string, std::string> &conf) {
auto bid = block_id_parser::parse(block_id);
storage_management_client client(bid.host, bid.management_port);
LOG(log_level::info) << "setup partition on " << bid.host << ":" << bid.management_port;
//LOG(log_level::info) << "Creating partition name : " << name << " on " << bid.host << ":" << bid.management_port;
Copy link
Member

Choose a reason for hiding this comment

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

Either put back the LOG statements, or remove them. Same thing for all the commented out log statements below..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@@ -42,6 +43,10 @@ void subscription_map::clear() {
std::lock_guard<std::mutex> lock{mtx_};
subs_.clear();
}
// TODO fix this
void subscription_map::send_failure() {
Copy link
Member

Choose a reason for hiding this comment

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

What is this supposed to do? Maybe put a more descriptive comment about what this function is supposed to do if you don't intend to implement it now..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

virtual ~partition() {
client_map_.send_failure();
client_map_.clear();
// TODO need to figure out if we want to notify an error to the subscripted clients after a delete of partition
Copy link
Member

Choose a reason for hiding this comment

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

We probably should -- create an issue unless we have one already

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, this refers to issue #46

Co-Authored-By: Anurag Khandelwal <anuragk@berkeley.edu>
@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/jiffy-prb/46/
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/jiffy-prb/47/
Test FAILed.

@charles-typ
Copy link
Collaborator Author

All fixed for this PR

@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/jiffy-prb/48/
Test FAILed.

@anuragkh anuragkh merged commit acf3fb8 into resource-disaggregation:new_arch Jun 25, 2019
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.

3 participants