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.

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

Fix group not found issue. #5013

Merged
merged 3 commits into from
May 27, 2024
Merged

Fix group not found issue. #5013

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b079975
Fix group not found issue.
KiterLuc May 27, 2024
636b325
Adding back comment.
KiterLuc May 27, 2024
0252d83
One small unstatus on serialization.
KiterLuc May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions tiledb/api/c_api/group/group_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,16 +114,15 @@ capi_return_t tiledb_group_open(
tiledb_group_handle_t* group, tiledb_query_type_t query_type) {
ensure_group_is_valid(group);

throw_if_not_ok(
group->group().open(static_cast<tiledb::sm::QueryType>(query_type)));
group->group().open(static_cast<tiledb::sm::QueryType>(query_type));

return TILEDB_OK;
}

capi_return_t tiledb_group_close(tiledb_group_handle_t* group) {
ensure_group_is_valid(group);

throw_if_not_ok(group->group().close());
group->group().close();

return TILEDB_OK;
}
Expand Down Expand Up @@ -270,8 +269,7 @@ capi_return_t tiledb_group_add_member(
name_optional = name;
}

throw_if_not_ok(
group->group().mark_member_for_addition(uri, relative, name_optional));
group->group().mark_member_for_addition(uri, relative, name_optional);

return TILEDB_OK;
}
Expand All @@ -281,7 +279,7 @@ capi_return_t tiledb_group_remove_member(
ensure_group_is_valid(group);
ensure_name_argument_is_valid(name_or_uri);

throw_if_not_ok(group->group().mark_member_for_removal(name_or_uri));
group->group().mark_member_for_removal(name_or_uri);

return TILEDB_OK;
}
Expand Down Expand Up @@ -463,8 +461,7 @@ capi_return_t tiledb_group_get_query_type(
ensure_output_pointer_is_valid(query_type);

// Get query_type
tiledb::sm::QueryType type;
throw_if_not_ok(group->group().get_query_type(&type));
tiledb::sm::QueryType type = group->group().get_query_type();

*query_type = static_cast<tiledb_query_type_t>(type);

Expand Down
17 changes: 10 additions & 7 deletions tiledb/sm/consolidator/group_meta_consolidator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,18 @@ Status GroupMetaConsolidator::consolidate(
auto group_uri = URI(group_name);
Group group_for_reads(
storage_manager_->resources(), group_uri, storage_manager_);
RETURN_NOT_OK(group_for_reads.open(
QueryType::READ, config_.timestamp_start_, config_.timestamp_end_));
group_for_reads.open(
QueryType::READ, config_.timestamp_start_, config_.timestamp_end_);

// Open group for writing
Group group_for_writes(
storage_manager_->resources(), group_uri, storage_manager_);
RETURN_NOT_OK_ELSE(
group_for_writes.open(QueryType::WRITE),
throw_if_not_ok(group_for_reads.close()));

try {
group_for_writes.open(QueryType::WRITE);
} catch (...) {
group_for_reads.close();
}

// Copy-assign the read metadata into the metadata of the group for writes
auto metadata_r = group_for_reads.metadata();
Expand All @@ -95,8 +98,8 @@ Status GroupMetaConsolidator::consolidate(
auto data = ss.str();

// Close groups
throw_if_not_ok(group_for_reads.close());
throw_if_not_ok(group_for_writes.close());
group_for_reads.close();
group_for_writes.close();

// Write vacuum file
throw_if_not_ok(resources_.vfs().write(vac_uri, data.c_str(), data.size()));
Expand Down
103 changes: 46 additions & 57 deletions tiledb/sm/group/group.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,16 +83,16 @@ Group::Group(
memory_tracker_->set_type(MemoryTrackerType::GROUP);
}

Status Group::open(
void Group::open(
QueryType query_type, uint64_t timestamp_start, uint64_t timestamp_end) {
// Checks
if (is_open_) {
return Status_GroupError("Cannot open group; Group already open");
throw GroupStatusException("Cannot open group; Group already open");
}

if (query_type != QueryType::READ && query_type != QueryType::WRITE &&
query_type != QueryType::MODIFY_EXCLUSIVE) {
return Status_GroupError("Cannot open group; Unsupported query type");
throw GroupStatusException("Cannot open group; Unsupported query type");
}

if (timestamp_end == UINT64_MAX) {
Expand Down Expand Up @@ -126,7 +126,7 @@ Status Group::open(
encryption_type_from_cfg = config_.get("sm.encryption_type", &found);
assert(found);
auto [st, et] = encryption_type_enum(encryption_type_from_cfg);
RETURN_NOT_OK(st);
throw_if_not_ok(st);
encryption_type = et.value();

if (!EncryptionKey::is_valid_key_length(
Expand All @@ -138,11 +138,11 @@ Status Group::open(
}

if (remote_ && encryption_type != EncryptionType::NO_ENCRYPTION)
return Status_GroupError(
throw GroupStatusException(
"Cannot open group; encrypted remote groups are not supported.");

// Copy the key bytes.
RETURN_NOT_OK(
throw_if_not_ok(
encryption_key_->set_key(encryption_type, encryption_key, key_length));

metadata_.clear();
Expand All @@ -151,14 +151,14 @@ Status Group::open(
if (remote_) {
auto rest_client = storage_manager_->rest_client();
if (rest_client == nullptr) {
return Status_GroupError(
throw GroupStatusException(
"Cannot open group; remote group with no REST client.");
}

// Set initial group details to be deserialized into
group_details_ = tdb::make_shared<GroupDetailsV2>(HERE(), group_uri_);

RETURN_NOT_OK(rest_client->post_group_from_rest(group_uri_, this));
throw_if_not_ok(rest_client->post_group_from_rest(group_uri_, this));
} else if (query_type == QueryType::READ) {
group_dir_ = make_shared<GroupDirectory>(
HERE(),
Expand Down Expand Up @@ -187,23 +187,18 @@ Status Group::open(

query_type_ = query_type;
is_open_ = true;

return Status::Ok();
}

Status Group::open(QueryType query_type) {
bool found = false;
RETURN_NOT_OK(config_.get<uint64_t>(
"sm.group.timestamp_start", &timestamp_start_, &found));
assert(found);
RETURN_NOT_OK(
config_.get<uint64_t>("sm.group.timestamp_end", &timestamp_end_, &found));
assert(found);
void Group::open(QueryType query_type) {
timestamp_start_ =
config_.get<uint64_t>("sm.group.timestamp_start", Config::must_find);
timestamp_end_ =
config_.get<uint64_t>("sm.group.timestamp_end", Config::must_find);

return Group::open(query_type, timestamp_start_, timestamp_end_);
Group::open(query_type, timestamp_start_, timestamp_end_);
}

Status Group::close_for_writes() {
void Group::close_for_writes() {
// Flush the group metadata
throw_if_not_ok(
unsafe_metadata()->store(resources_, group_uri(), *encryption_key()));
Expand All @@ -218,13 +213,12 @@ Status Group::close_for_writes() {
group_detail_uri,
*encryption_key()));
}
return Status::Ok();
}

Status Group::close() {
void Group::close() {
// Check if group is open
if (!is_open_)
return Status::Ok();
return;

if (remote_) {
// Update group metadata for write queries if metadata was written by the
Expand All @@ -237,28 +231,28 @@ Status Group::close() {
metadata_loaded_ = true;
auto rest_client = storage_manager_->rest_client();
if (rest_client == nullptr)
return Status_GroupError(
throw GroupStatusException(
"Error closing group; remote group with no REST client.");
RETURN_NOT_OK(
throw_if_not_ok(
rest_client->put_group_metadata_to_rest(group_uri_, this));
}
if (!members_to_modify().empty()) {
auto rest_client = storage_manager_->rest_client();
if (rest_client == nullptr)
return Status_GroupError(
throw GroupStatusException(
"Error closing group; remote group with no REST client.");
RETURN_NOT_OK(rest_client->patch_group_to_rest(group_uri_, this));
throw_if_not_ok(rest_client->patch_group_to_rest(group_uri_, this));
}
}
// Storage manager does not own the group schema for remote groups.
} else {
if (query_type_ == QueryType::READ) {
RETURN_NOT_OK(close_for_reads());
close_for_reads();
} else if (
query_type_ == QueryType::WRITE ||
query_type_ == QueryType::MODIFY_EXCLUSIVE) {
try {
throw_if_not_ok(close_for_writes());
close_for_writes();
} catch (StatusException& exc) {
std::string msg = exc.what();
msg += " : Was storage for the group moved or deleted before closing?";
Expand All @@ -271,7 +265,6 @@ Status Group::close() {
metadata_loaded_ = false;
is_open_ = false;
clear();
return Status::Ok();
}

bool Group::is_open() const {
Expand All @@ -290,15 +283,13 @@ const shared_ptr<GroupDetails> Group::group_details() const {
return group_details_;
}

Status Group::get_query_type(QueryType* query_type) const {
QueryType Group::get_query_type() const {
// Error if the group is not open
if (!is_open_)
return LOG_STATUS(
Status_GroupError("Cannot get query_type; Group is not open"));

*query_type = query_type_;
if (!is_open_) {
throw GroupStatusException("Cannot get query_type; Group is not open");
}

return Status::Ok();
return query_type_;
}

void Group::delete_group(const URI& uri, bool recursive) {
Expand Down Expand Up @@ -334,7 +325,7 @@ void Group::delete_group(const URI& uri, bool recursive) {
Array::delete_array(resources_, member_uri);
} else if (member->type() == ObjectType::GROUP) {
Group group_rec(resources_, member_uri, storage_manager_);
throw_if_not_ok(group_rec.open(QueryType::MODIFY_EXCLUSIVE));
group_rec.open(QueryType::MODIFY_EXCLUSIVE);
group_rec.delete_group(member_uri, true);
}
}
Expand Down Expand Up @@ -366,7 +357,7 @@ void Group::delete_group(const URI& uri, bool recursive) {
group_details_->clear();

// Close the deleted group
throw_if_not_ok(this->close());
this->close();
}

void Group::delete_metadata(const char* key) {
Expand Down Expand Up @@ -551,46 +542,44 @@ void Group::delete_member(const shared_ptr<GroupMember> group_member) {
group_details_->delete_member(group_member);
}

Status Group::mark_member_for_addition(
void Group::mark_member_for_addition(
const URI& group_member_uri,
const bool& relative,
std::optional<std::string>& name) {
std::lock_guard<std::mutex> lck(mtx_);
// Check if group is open
if (!is_open_) {
return Status_GroupError("Cannot add member; Group is not open");
throw GroupStatusException("Cannot add member; Group is not open");
}

// Check mode
if (query_type_ != QueryType::WRITE &&
query_type_ != QueryType::MODIFY_EXCLUSIVE) {
return Status_GroupError(
throw GroupStatusException(
"Cannot get member; Group was not opened in write or modify_exclusive "
"mode");
}
group_details_->mark_member_for_addition(
group_member_uri, relative, name, storage_manager_);
return Status::Ok();
}

Status Group::mark_member_for_removal(const std::string& name) {
void Group::mark_member_for_removal(const std::string& name) {
std::lock_guard<std::mutex> lck(mtx_);
// Check if group is open
if (!is_open_) {
return Status_GroupError(
throw GroupStatusException(
"Cannot mark member for removal; Group is not open");
}

// Check mode
if (query_type_ != QueryType::WRITE &&
query_type_ != QueryType::MODIFY_EXCLUSIVE) {
return Status_GroupError(
throw GroupStatusException(
"Cannot get member; Group was not opened in write or modify_exclusive "
"mode");
}

group_details_->mark_member_for_removal(name);
return Status::Ok();
}

const std::vector<shared_ptr<GroupMember>>& Group::members_to_modify() const {
Expand Down Expand Up @@ -628,12 +617,12 @@ uint64_t Group::member_count() const {
std::lock_guard<std::mutex> lck(mtx_);
// Check if group is open
if (!is_open_) {
throw Status_GroupError("Cannot get member count; Group is not open");
throw GroupStatusException("Cannot get member count; Group is not open");
}

// Check mode
if (query_type_ != QueryType::READ) {
throw Status_GroupError(
throw GroupStatusException(
"Cannot get member; Group was not opened in read mode");
}

Expand All @@ -646,12 +635,12 @@ tuple<std::string, ObjectType, optional<std::string>> Group::member_by_index(

// Check if group is open
if (!is_open_) {
throw Status_GroupError("Cannot get member by index; Group is not open");
throw GroupStatusException("Cannot get member by index; Group is not open");
}

// Check mode
if (query_type_ != QueryType::READ) {
throw Status_GroupError(
throw GroupStatusException(
"Cannot get member; Group was not opened in read mode");
}

Expand All @@ -664,12 +653,12 @@ Group::member_by_name(const std::string& name) {

// Check if group is open
if (!is_open_) {
throw Status_GroupError("Cannot get member by name; Group is not open");
throw GroupStatusException("Cannot get member by name; Group is not open");
}

// Check mode
if (query_type_ != QueryType::READ) {
throw Status_GroupError(
throw GroupStatusException(
"Cannot get member; Group was not opened in read mode");
}

Expand Down Expand Up @@ -701,12 +690,12 @@ std::string Group::dump(
}

Group group_rec(resources_, member_uri, storage_manager_);
auto st = group_rec.open(QueryType::READ);
if (st.ok()) {
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the actual fix.

group_rec.open(QueryType::READ);
ss << std::endl;
ss << group_rec.dump(indent_size, num_indents + 2, recursive, false);
throw_if_not_ok(group_rec.close());
} else {
group_rec.close();
} catch (GroupNotFoundException&) {
// If the group no longer exists in storage it will be listed but we
// won't be able to dump its members
ss << " (does not exist)" << std::endl;
Expand Down
Loading
Loading