Skip to content

Commit 8c701dd

Browse files
shaunrd0shaunrd0
authored
Update S3 to inherit from FilesystemBase. (#4608)
This updates S3 to inherit from FilesystemBase. Since other cloud backends will inherit from FilesystemBase in the future, this adds common cloud functions to the base class and stubs them out as no-ops in Posix. I also reorganized definitions to match declaration order for both Posix and S3 in my last two commits. --- TYPE: NO_HISTORY DESC: Update S3 to inherit from FilesystemBase.
1 parent 2085d8c commit 8c701dd

File tree

12 files changed

+1154
-969
lines changed

12 files changed

+1154
-969
lines changed

‎test/src/unit-s3-no-multipart.cc‎

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -62,33 +62,30 @@ struct S3DirectFx {
6262

6363
S3DirectFx::S3DirectFx() {
6464
// Create bucket
65-
bool exists;
66-
REQUIRE(s3_.is_bucket(S3_BUCKET, &exists).ok());
65+
bool exists = s3_.is_bucket(S3_BUCKET);
6766
if (exists)
68-
REQUIRE(s3_.remove_bucket(S3_BUCKET).ok());
67+
REQUIRE_NOTHROW(s3_.remove_bucket(S3_BUCKET));
6968

70-
REQUIRE(s3_.is_bucket(S3_BUCKET, &exists).ok());
69+
exists = s3_.is_bucket(S3_BUCKET);
7170
REQUIRE(!exists);
72-
REQUIRE(s3_.create_bucket(S3_BUCKET).ok());
71+
REQUIRE_NOTHROW(s3_.create_bucket(S3_BUCKET));
7372

7473
// Check if bucket is empty
75-
bool is_empty;
76-
REQUIRE(s3_.is_empty_bucket(S3_BUCKET, &is_empty).ok());
74+
bool is_empty = s3_.is_empty_bucket(S3_BUCKET);
7775
CHECK(is_empty);
7876
}
7977

8078
S3DirectFx::~S3DirectFx() {
8179
// Empty bucket
82-
bool is_empty;
83-
CHECK(s3_.is_empty_bucket(S3_BUCKET, &is_empty).ok());
80+
bool is_empty = s3_.is_empty_bucket(S3_BUCKET);
8481
if (!is_empty) {
85-
CHECK(s3_.empty_bucket(S3_BUCKET).ok());
86-
CHECK(s3_.is_empty_bucket(S3_BUCKET, &is_empty).ok());
82+
CHECK_NOTHROW(s3_.empty_bucket(S3_BUCKET));
83+
is_empty = s3_.is_empty_bucket(S3_BUCKET);
8784
CHECK(is_empty);
8885
}
8986

9087
// Delete bucket
91-
CHECK(s3_.remove_bucket(S3_BUCKET).ok());
88+
CHECK_NOTHROW(s3_.remove_bucket(S3_BUCKET));
9289
CHECK(s3_.disconnect().ok());
9390
}
9491

@@ -124,10 +121,12 @@ TEST_CASE_METHOD(
124121

125122
// Write to two files
126123
auto largefile = TEST_DIR + "largefile";
127-
CHECK(s3_.write(URI(largefile), write_buffer, buffer_size).ok());
128-
CHECK(s3_.write(URI(largefile), write_buffer_small, buffer_size_small).ok());
124+
CHECK_NOTHROW(s3_.write(URI(largefile), write_buffer, buffer_size));
125+
CHECK_NOTHROW(
126+
s3_.write(URI(largefile), write_buffer_small, buffer_size_small));
129127
auto smallfile = TEST_DIR + "smallfile";
130-
CHECK(s3_.write(URI(smallfile), write_buffer_small, buffer_size_small).ok());
128+
CHECK_NOTHROW(
129+
s3_.write(URI(smallfile), write_buffer_small, buffer_size_small));
131130

132131
// Before flushing, the files do not exist
133132
bool exists = false;
@@ -156,7 +155,8 @@ TEST_CASE_METHOD(
156155
// Read from the beginning
157156
auto read_buffer = new char[26];
158157
uint64_t bytes_read = 0;
159-
CHECK(s3_.read(URI(largefile), 0, read_buffer, 26, 0, &bytes_read).ok());
158+
CHECK_NOTHROW(
159+
s3_.read_impl(URI(largefile), 0, read_buffer, 26, 0, &bytes_read));
160160
assert(26 == bytes_read);
161161
bool allok = true;
162162
for (int i = 0; i < 26; i++) {
@@ -168,7 +168,8 @@ TEST_CASE_METHOD(
168168
CHECK(allok);
169169

170170
// Read from a different offset
171-
CHECK(s3_.read(URI(largefile), 11, read_buffer, 26, 0, &bytes_read).ok());
171+
CHECK_NOTHROW(
172+
s3_.read_impl(URI(largefile), 11, read_buffer, 26, 0, &bytes_read));
172173
assert(26 == bytes_read);
173174
allok = true;
174175
for (int i = 0; i < 26; i++) {
@@ -182,7 +183,7 @@ TEST_CASE_METHOD(
182183
// Try to write 11 MB file, should fail with given buffer configuration
183184
auto badfile = TEST_DIR + "badfile";
184185
auto badbuffer = (char*)malloc(11000000);
185-
CHECK(!(s3_.write(URI(badfile), badbuffer, 11000000).ok()));
186+
CHECK_THROWS((s3_.write(URI(badfile), badbuffer, 11000000)));
186187
}
187188

188189
TEST_CASE_METHOD(
@@ -199,9 +200,8 @@ TEST_CASE_METHOD(
199200
tiledb::sm::S3 s3{&g_helper_stats, &thread_pool_, cfg};
200201
auto uri = URI(TEST_DIR + "writefailure");
201202

202-
// This is a buffered write, which is why it returns ok.
203-
auto st = s3.write(uri, "Validate s3 custom headers", 26);
204-
REQUIRE(st.ok());
203+
// This is a buffered write, which is why it should not throw.
204+
CHECK_NOTHROW(s3.write(uri, "Validate s3 custom headers", 26));
205205

206206
auto matcher = Catch::Matchers::ContainsSubstring(
207207
"The Content-Md5 you specified is not valid.");

‎test/src/unit-s3.cc‎

Lines changed: 38 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -63,33 +63,30 @@ struct S3Fx {
6363

6464
S3Fx::S3Fx() {
6565
// Create bucket
66-
bool exists;
67-
REQUIRE(s3_.is_bucket(S3_BUCKET, &exists).ok());
66+
bool exists = s3_.is_bucket(S3_BUCKET);
6867
if (exists)
69-
REQUIRE(s3_.remove_bucket(S3_BUCKET).ok());
68+
REQUIRE_NOTHROW(s3_.remove_bucket(S3_BUCKET));
7069

71-
REQUIRE(s3_.is_bucket(S3_BUCKET, &exists).ok());
70+
exists = s3_.is_bucket(S3_BUCKET);
7271
REQUIRE(!exists);
73-
REQUIRE(s3_.create_bucket(S3_BUCKET).ok());
72+
REQUIRE_NOTHROW(s3_.create_bucket(S3_BUCKET));
7473

7574
// Check if bucket is empty
76-
bool is_empty;
77-
REQUIRE(s3_.is_empty_bucket(S3_BUCKET, &is_empty).ok());
75+
bool is_empty = s3_.is_empty_bucket(S3_BUCKET);
7876
CHECK(is_empty);
7977
}
8078

8179
S3Fx::~S3Fx() {
8280
// Empty bucket
83-
bool is_empty;
84-
CHECK(s3_.is_empty_bucket(S3_BUCKET, &is_empty).ok());
81+
bool is_empty = s3_.is_empty_bucket(S3_BUCKET);
8582
if (!is_empty) {
86-
CHECK(s3_.empty_bucket(S3_BUCKET).ok());
87-
CHECK(s3_.is_empty_bucket(S3_BUCKET, &is_empty).ok());
83+
CHECK_NOTHROW(s3_.empty_bucket(S3_BUCKET));
84+
is_empty = s3_.is_empty_bucket(S3_BUCKET);
8885
CHECK(is_empty);
8986
}
9087

9188
// Delete bucket
92-
CHECK(s3_.remove_bucket(S3_BUCKET).ok());
89+
CHECK_NOTHROW(s3_.remove_bucket(S3_BUCKET));
9390
CHECK(s3_.disconnect().ok());
9491
}
9592

@@ -118,10 +115,12 @@ TEST_CASE_METHOD(S3Fx, "Test S3 filesystem, file I/O", "[s3]") {
118115

119116
// Write to two files
120117
auto largefile = TEST_DIR + "largefile";
121-
CHECK(s3_.write(URI(largefile), write_buffer, buffer_size).ok());
122-
CHECK(s3_.write(URI(largefile), write_buffer_small, buffer_size_small).ok());
118+
CHECK_NOTHROW(s3_.write(URI(largefile), write_buffer, buffer_size));
119+
CHECK_NOTHROW(
120+
s3_.write(URI(largefile), write_buffer_small, buffer_size_small));
123121
auto smallfile = TEST_DIR + "smallfile";
124-
CHECK(s3_.write(URI(smallfile), write_buffer_small, buffer_size_small).ok());
122+
CHECK_NOTHROW(
123+
s3_.write(URI(smallfile), write_buffer_small, buffer_size_small));
125124

126125
// Before flushing, the files do not exist
127126
bool exists = false;
@@ -150,7 +149,8 @@ TEST_CASE_METHOD(S3Fx, "Test S3 filesystem, file I/O", "[s3]") {
150149
// Read from the beginning
151150
auto read_buffer = new char[26];
152151
uint64_t bytes_read = 0;
153-
CHECK(s3_.read(URI(largefile), 0, read_buffer, 26, 0, &bytes_read).ok());
152+
CHECK_NOTHROW(
153+
s3_.read_impl(URI(largefile), 0, read_buffer, 26, 0, &bytes_read));
154154
CHECK(26 == bytes_read);
155155
bool allok = true;
156156
for (int i = 0; i < 26; i++) {
@@ -162,7 +162,8 @@ TEST_CASE_METHOD(S3Fx, "Test S3 filesystem, file I/O", "[s3]") {
162162
CHECK(allok);
163163

164164
// Read from a different offset
165-
CHECK(s3_.read(URI(largefile), 11, read_buffer, 26, 0, &bytes_read).ok());
165+
CHECK_NOTHROW(
166+
s3_.read_impl(URI(largefile), 11, read_buffer, 26, 0, &bytes_read));
166167
CHECK(26 == bytes_read);
167168
allok = true;
168169
for (int i = 0; i < 26; i++) {
@@ -188,7 +189,7 @@ TEST_CASE_METHOD(S3Fx, "Test S3 multiupload abort path", "[s3]") {
188189
// Write one large file, the write will fail
189190
auto largefile =
190191
TEST_DIR + "failed_largefile_" + std::to_string(nth_failure);
191-
CHECK(!s3_.write(URI(largefile), write_buffer, buffer_size).ok());
192+
CHECK_THROWS(s3_.write(URI(largefile), write_buffer, buffer_size));
192193

193194
// Before flushing, the file does not exist
194195
bool exists = false;
@@ -240,18 +241,16 @@ TEST_CASE_METHOD(S3Fx, "Test S3 use BucketCannedACL", "[s3]") {
240241
tiledb::sm::S3 s3_{&g_helper_stats, &thread_pool_, config};
241242

242243
// Create bucket
243-
bool exists;
244-
REQUIRE(s3_.is_bucket(S3_BUCKET, &exists).ok());
244+
bool exists = s3_.is_bucket(S3_BUCKET);
245245
if (exists)
246-
REQUIRE(s3_.remove_bucket(S3_BUCKET).ok());
246+
REQUIRE_NOTHROW(s3_.remove_bucket(S3_BUCKET));
247247

248-
REQUIRE(s3_.is_bucket(S3_BUCKET, &exists).ok());
248+
exists = s3_.is_bucket(S3_BUCKET);
249249
REQUIRE(!exists);
250-
REQUIRE(s3_.create_bucket(S3_BUCKET).ok());
250+
REQUIRE_NOTHROW(s3_.create_bucket(S3_BUCKET));
251251

252252
// Check if bucket is empty
253-
bool is_empty;
254-
REQUIRE(s3_.is_empty_bucket(S3_BUCKET, &is_empty).ok());
253+
bool is_empty = s3_.is_empty_bucket(S3_BUCKET);
255254
CHECK(is_empty);
256255

257256
CHECK(s3_.disconnect().ok());
@@ -295,30 +294,29 @@ TEST_CASE_METHOD(S3Fx, "Test S3 use Bucket/Object CannedACL", "[s3]") {
295294
auto file6 = TEST_DIR + "file6";
296295

297296
// Check that bucket is empty
298-
bool is_empty;
299-
CHECK(s3_.is_empty_bucket(S3_BUCKET, &is_empty).ok());
297+
bool is_empty = s3_.is_empty_bucket(S3_BUCKET);
300298
CHECK(is_empty);
301299

302300
// Continue building the hierarchy
303301
bool exists = false;
304-
CHECK(s3_.touch(URI(file1)).ok());
302+
CHECK_NOTHROW(s3_.touch(URI(file1)));
305303
CHECK(s3_.is_object(URI(file1), &exists).ok());
306304
CHECK(exists);
307-
CHECK(s3_.touch(URI(file2)).ok());
305+
CHECK_NOTHROW(s3_.touch(URI(file2)));
308306
CHECK(s3_.is_object(URI(file2), &exists).ok());
309307
CHECK(exists);
310-
CHECK(s3_.touch(URI(file3)).ok());
308+
CHECK_NOTHROW(s3_.touch(URI(file3)));
311309
CHECK(s3_.is_object(URI(file3), &exists).ok());
312310
CHECK(exists);
313-
CHECK(s3_.touch(URI(file4)).ok());
311+
CHECK_NOTHROW(s3_.touch(URI(file4)));
314312
CHECK(s3_.is_object(URI(file4), &exists).ok());
315313
CHECK(exists);
316-
CHECK(s3_.touch(URI(file5)).ok());
314+
CHECK_NOTHROW(s3_.touch(URI(file5)));
317315
CHECK(s3_.is_object(URI(file5), &exists).ok());
318316
CHECK(exists);
319317

320318
// Check that the bucket is not empty
321-
CHECK(s3_.is_empty_bucket(S3_BUCKET, &is_empty).ok());
319+
is_empty = s3_.is_empty_bucket(S3_BUCKET);
322320
CHECK(!is_empty);
323321

324322
// Check invalid file
@@ -361,7 +359,7 @@ TEST_CASE_METHOD(S3Fx, "Test S3 use Bucket/Object CannedACL", "[s3]") {
361359
CHECK(paths.size() == 5);
362360

363361
// Move directory
364-
CHECK(s3_.move_dir(URI(dir), URI(dir2)).ok());
362+
CHECK_NOTHROW(s3_.move_dir(URI(dir), URI(dir2)));
365363
CHECK(s3_.is_dir(URI(dir), &is_dir).ok());
366364
CHECK(!is_dir);
367365
CHECK(s3_.is_dir(URI(dir2), &is_dir).ok());
@@ -376,7 +374,7 @@ TEST_CASE_METHOD(S3Fx, "Test S3 use Bucket/Object CannedACL", "[s3]") {
376374
CHECK(!exists);
377375

378376
// Remove directories
379-
CHECK(s3_.remove_dir(URI(dir2)).ok());
377+
CHECK_NOTHROW(s3_.remove_dir(URI(dir2)));
380378
CHECK(s3_.is_object(URI(file1), &exists).ok());
381379
CHECK(!exists);
382380
CHECK(s3_.is_object(URI(file2), &exists).ok());
@@ -394,18 +392,16 @@ TEST_CASE_METHOD(S3Fx, "Test S3 use Bucket/Object CannedACL", "[s3]") {
394392
tiledb::sm::S3 s3_{&g_helper_stats, &thread_pool_, config};
395393

396394
// Create bucket
397-
bool exists;
398-
REQUIRE(s3_.is_bucket(S3_BUCKET, &exists).ok());
395+
bool exists = s3_.is_bucket(S3_BUCKET);
399396
if (exists)
400-
REQUIRE(s3_.remove_bucket(S3_BUCKET).ok());
397+
REQUIRE_NOTHROW(s3_.remove_bucket(S3_BUCKET));
401398

402-
REQUIRE(s3_.is_bucket(S3_BUCKET, &exists).ok());
399+
exists = s3_.is_bucket(S3_BUCKET);
403400
REQUIRE(!exists);
404-
REQUIRE(s3_.create_bucket(S3_BUCKET).ok());
401+
REQUIRE_NOTHROW(s3_.create_bucket(S3_BUCKET));
405402

406403
// Check if bucket is empty
407-
bool is_empty;
408-
REQUIRE(s3_.is_empty_bucket(S3_BUCKET, &is_empty).ok());
404+
bool is_empty = s3_.is_empty_bucket(S3_BUCKET);
409405
CHECK(is_empty);
410406

411407
exercise_object_canned_acl();

‎test/src/unit-vfs.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ TEST_CASE("VFS: Test long local paths", "[vfs]") {
152152
require_tiledb_ok(vfs.is_file(testfile, &exists));
153153

154154
// Creating the file is not
155-
require_tiledb_err(vfs.touch(testfile));
155+
REQUIRE_THROWS(vfs.touch(testfile));
156156
}
157157

158158
// Creating the URI is invalid on Win32 (failure to canonicalize path)

‎test/support/src/vfs_helpers.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -862,15 +862,15 @@ class S3Test : public VFSTestBase, protected tiledb::sm::S3_within_VFS {
862862
: VFSTestBase(test_tree, "s3://")
863863
, S3_within_VFS(&tiledb::test::g_helper_stats, &io_, vfs_.config()) {
864864
#ifdef HAVE_S3
865-
s3().create_bucket(temp_dir_).ok();
865+
s3().create_bucket(temp_dir_);
866866
for (size_t i = 1; i <= test_tree_.size(); i++) {
867867
sm::URI path = temp_dir_.join_path("subdir_" + std::to_string(i));
868868
// VFS::create_dir is a no-op for S3; Just create objects.
869869
for (size_t j = 1; j <= test_tree_[i - 1]; j++) {
870870
auto object_uri = path.join_path("test_file_" + std::to_string(j));
871-
s3().touch(object_uri).ok();
871+
s3().touch(object_uri);
872872
std::string data(j * 10, 'a');
873-
s3().write(object_uri, data.data(), data.size()).ok();
873+
s3().write(object_uri, data.data(), data.size());
874874
s3().flush_object(object_uri).ok();
875875
expected_results().emplace_back(object_uri.to_string(), data.size());
876876
}

0 commit comments

Comments
 (0)