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

Optimize VFS::touch and fix defects and race conditions. #5285

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions test/src/unit-vfs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,12 @@ TEST_CASE("VFS: test ls_with_sizes", "[vfs][ls-with-sizes]") {
// Directories don't get a size
REQUIRE(children[1].file_size() == 0);

// Touch does not overwrite an existing file.
require_tiledb_ok(vfs_ls.touch(URI(subdir_file)));
uint64_t size;
require_tiledb_ok(vfs_ls.file_size(URI(subdir_file), &size));
REQUIRE(size == 6);

// Clean up
require_tiledb_ok(vfs_ls.remove_dir(URI(path)));
}
Expand Down
4 changes: 3 additions & 1 deletion tiledb/api/c_api/vfs/vfs_api_external.h
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,9 @@ TILEDB_EXPORT capi_return_t tiledb_vfs_fh_is_closed(
tiledb_ctx_t* ctx, tiledb_vfs_fh_t* fh, int32_t* is_closed) TILEDB_NOEXCEPT;

/**
* Touches a file, i.e., creates a new empty file.
* Touches a file, i.e., creates a new empty file if it does not already exist.
*
* The access timestamps of the file are not guaranteed to change.
*
* **Example:**
*
Expand Down
3 changes: 2 additions & 1 deletion tiledb/sm/cpp_api/vfs.h
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,8 @@ class VFS {
ctx.ptr().get(), vfs_.get(), old_uri.c_str(), new_uri.c_str()));
}

/** Touches a file with the input URI, i.e., creates a new empty file. */
/** Touches a file with the input URI, i.e., creates a new empty file if it
* does not already exist. */
void touch(const std::string& uri) const {
auto& ctx = ctx_.get();
ctx.handle_error(
Expand Down
17 changes: 10 additions & 7 deletions tiledb/sm/filesystem/azure.cc
Original file line number Diff line number Diff line change
Expand Up @@ -860,21 +860,24 @@ Status Azure::touch(const URI& uri) const {
"Cannot create file; URI is a directory: " + uri.to_string())));
}

bool is_blob;
RETURN_NOT_OK(this->is_blob(uri, &is_blob));
if (is_blob) {
return Status::Ok();
}

std::string container_name;
std::string blob_path;
RETURN_NOT_OK(parse_azure_uri(uri, &container_name, &blob_path));

try {
::Azure::Core::IO::MemoryBodyStream stream(nullptr, 0);
::Azure::Storage::Blobs::UploadBlockBlobOptions options;
// Set condition that the object does not exist.
options.AccessConditions.IfNoneMatch = ::Azure::ETag::Any();

c.GetBlobContainerClient(container_name)
.GetBlockBlobClient(blob_path)
.UploadFrom(nullptr, 0);
.Upload(stream, options);
} catch (const ::Azure::Storage::StorageException& e) {
if (e.StatusCode == ::Azure::Core::Http::HttpStatusCode::Conflict) {
// Blob already exists.
return Status::Ok();
}
return LOG_STATUS(Status_AzureError(
"Touch blob failed on: " + uri.to_string() + "; " + e.Message));
}
Expand Down
14 changes: 12 additions & 2 deletions tiledb/sm/filesystem/gcs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -769,10 +769,20 @@ Status GCS::touch(const URI& uri) const {
RETURN_NOT_OK(parse_gcs_uri(uri, &bucket_name, &object_path));

google::cloud::StatusOr<google::cloud::storage::ObjectMetadata>
object_metadata = client_->InsertObject(bucket_name, object_path, "");
object_metadata = client_->InsertObject(
bucket_name,
object_path,
"",
// An if-generation-match value of zero makes the request succeed only
// if the object does not exist.
// https://cloud.google.com/storage/docs/request-preconditions#special-case
google::cloud::storage::IfGenerationMatch(0));

if (!object_metadata.ok()) {
const google::cloud::Status status = object_metadata.status();
const google::cloud::Status& status = object_metadata.status();
if (status.code() == google::cloud::StatusCode::kFailedPrecondition) {
return Status::Ok();
}

return LOG_STATUS(Status_GCSError(std::string(
"Touch object failed on: " + uri.to_string() + " (" + status.message() +
Expand Down
8 changes: 4 additions & 4 deletions tiledb/sm/filesystem/win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,6 @@ Status Win::create_dir(const std::string& path) const {
}

Status Win::touch(const std::string& filename) const {
if (is_file(filename)) {
return Status::Ok();
}

HANDLE file_h = CreateFile(
filename.c_str(),
GENERIC_WRITE,
Expand All @@ -178,6 +174,10 @@ Status Win::touch(const std::string& filename) const {
tiledb::common::ScopedExecutor onexit1(closefileonexit);
if (file_h == INVALID_HANDLE_VALUE) {
auto gle = GetLastError();
if (gle == ERROR_FILE_EXISTS) {
// Do not fail if the file already exists.
return Status::Ok();
}
return LOG_STATUS(Status_IOError(
std::string("Failed to create file '") + filename + " (" +
get_last_error_msg(gle, "CreateFile") + ")'"));
Expand Down
Loading