Add support for remote global order writes #3393

robertbindar · 2022-07-28T13:37:40Z

TYPE: FEATURE
DESC: Add support for remote global order writes

shortcut-integration · 2022-07-28T13:37:42Z

This pull request has been linked to Shortcut Story #10668: Implement serialization for global order writes..

KiterLuc · 2022-07-29T07:07:08Z

test/src/unit-capi-array.cc

+  // rc = tiledb_query_finalize(ctx_, query);
+  // CHECK(rc == TILEDB_OK);
+  // TODO: Bug, wrong results
+  submit_serialized_query(ctx_, query);


In create_dense_vector, set the capacity to 10 on the array schema. Should fix this case.

KiterLuc

Initial feedback... Amazing progress!!

KiterLuc · 2022-08-24T06:33:13Z

test/src/helpers.h

+    Query* query,
+    bool clientside);
+
+int serialize_query(


Doxygen doc.

KiterLuc · 2022-08-24T06:33:45Z

test/src/helpers.h

+void submit_and_finalize_serialized_query(
+    tiledb_ctx_t* ctx, tiledb_query_t* query);
+
+void submit_and_finalize_serialized_query(const Context& ctx, Query& query);


NIT: line break between function and doc for the next.

KiterLuc · 2022-08-24T06:34:41Z

test/src/unit-capi-any.cc

-  REQUIRE(rc == TILEDB_OK);
-  rc = tiledb_query_finalize(ctx, query);  // Second time must create no problem
-  REQUIRE(rc == TILEDB_OK);
+  // rc = tiledb_query_submit(ctx, query);


Remove commented lines everywhere.

KiterLuc · 2022-08-24T06:36:05Z

test/src/helpers.cc

@@ -1216,6 +1216,223 @@ void check_counts(span<T> vals, std::vector<uint64_t> expected) {
    CHECK(counts[i] == expected[i]);
  }
 }
+void serialize_query(


Are these just moving of the code in test/src/unit-capi-serialized_queries.cc?

KiterLuc · 2022-08-24T06:37:33Z

test/src/unit-capi-serialized_queries.cc

@@ -54,6 +54,10 @@
 using namespace tiledb;
 using ResultSetType = std::map<std::string, std::any>;

+using tiledb::test::allocate_query_buffers;


Can we change to just using tiledb::test?

I've noticed this could be cleaned up in other files as well.

KiterLuc · 2022-08-24T07:23:06Z

tiledb/sm/fragment/fragment_metadata.h

@@ -815,6 +823,141 @@ class FragmentMetadata {
   */
  const shared_ptr<const ArraySchema>& array_schema() const;



As this is only used for serialization, instead of writing all those accessors, it might be better to create a new object that will contain references to all of those. That way when we move to the new serialization standard, we don't have to come in and remove all those unused methods.

I'm also not a big fan of having reference accessors that are used to modify the internal state of the object, I know this might be a lot of work but maybe it would be a worthy investment to make a constructor taking in everything needed from deserialization to create a fully functional FragmentMetadata object? This would get rid of the need for set_schema_name and set_dense for example.

KiterLuc · 2022-08-24T07:55:49Z

tiledb/sm/query/domain_buffer.h

@@ -155,6 +190,16 @@ class SingleCoord {
    return &qb_[d];
  }

+  inline const std::vector<std::vector<uint8_t>>& get_coords() const {


Whatever we decide for the fragment metadata accessors should be done here too.

KiterLuc · 2022-08-24T08:05:17Z

tiledb/sm/filesystem/vfs.cc

@@ -1786,5 +1786,85 @@ Status VFS::write(const URI& uri, const void* buffer, uint64_t buffer_size) {
      Status_VFSError("Unsupported URI schemes: " + uri.to_string()));
 }

+std::pair<Status, std::optional<VFS::MultiPartUploadState>>


Any chance this method could just throw and not return a pair?

KiterLuc · 2022-08-24T08:06:47Z

tiledb/sm/filesystem/s3.h

@@ -671,6 +707,8 @@ class S3 {
  /** If !NOT_SET assign to bucket requests supporting SetACL() */
  Aws::S3::Model::BucketCannedACL bucket_canned_acl_;

+  friend class VFS;


Could you comment why the friend declaration is needed?

KiterLuc · 2022-08-24T08:08:05Z

tiledb/sm/query/query.cc

+
+  if (array_->is_remote()) {
+    auto rest_client = storage_manager_->rest_client();
+    if (rest_client == nullptr)


Remove one line ifs.

global write tests to serializated roundtrip submits/finalize

…zation

…oute

also add cpp api for submit_and_finalize for easier testing

S3 bucket, key and status needs to be serialized as well. Code simplification. Getting s3 multipart state can be done with a single accessor function

…alized

When cloud executors die, context gets destructed, so S3::disconnect tries to complete multipart request

…tion

The coords_info_ markers need to be reset for remote global order writes too, otherwise the client fails a check for equal size input coords buffer between submits

query_to_capnp doesn't handle well uint32 offset buffers, so tiledb_buffer_list_flatten ends up overflowing the offset buffers

for remote global order writes. Standalone S3 global order writes should still buffer as normal

ihnorton

Some misc comments for follow-up.

ihnorton · 2022-09-28T15:08:32Z

tiledb/sm/serialization/tiledb-rest.capnp

+  status@4 :Text;
+  completedParts@5 :List(CompletedPart);
+}
+struct CompletedPart {


Rename to S3CompletedPart?

ihnorton · 2022-09-28T15:23:44Z

tiledb/sm/query/query.h

@@ -361,6 +361,9 @@ class Query {
  /** Returns the array schema. */
  const ArraySchema& array_schema() const;

+  /** Returns the array schema as a shared_ptr */
+  const std::shared_ptr<const ArraySchema> array_schema_shared() const;


This is a big code smell, in the future we should refactor rather than adding an accessor like this. The ownership here is problematic because the lifetime of a query has nothing to do with a fragment metadata (where this is used AFAICT).

ihnorton · 2022-09-28T15:28:55Z

tiledb/sm/query/query.cc

+  return Status::Ok();
+}
+
+Status Query::check_buffer_multipart_size() const {


Do we have test coverage for error handling in this situation? I don't think this check is sufficient, because a chunk may be smaller than 5 MB after compression.

ihnorton · 2022-09-28T15:31:03Z

tiledb/sm/query/query.cc

-    if (strategy_ != nullptr)
+
+    // When executed from serialization, resetting the strategy will delete
+    // the fragment for global order writes. We need to prevent this, thus


Do you mind to clarify what fragment is this referring to?

ihnorton · 2022-09-28T15:55:52Z

tiledb/sm/query/query.cc

@@ -2416,6 +2541,27 @@ tuple<Status, optional<bool>> Query::non_overlapping_ranges() {
  return subarray_.non_overlapping_ranges(storage_manager_->compute_tp());
 }

+bool Query::is_dense() const {
+  return !coords_info_.has_coords_;


Can't we check the schema here instead? This is going to be wrong for dense queries with coordinate return for matching cells.

ihnorton · 2022-09-28T16:03:11Z

tiledb/sm/query/writers/global_order_writer.cc

+    // If there is no entry for this uri, probably multipart upload is disabled
+    // or no write was issued so far
+    if (!state.has_value()) {
+      return {Status::Ok(), {}};


Shouldn't we continue here?

ihnorton · 2022-09-28T17:25:29Z

tiledb/sm/filesystem/vfs.cc

+  return {Status::Ok(), nullopt};
+}
+
+Status VFS::set_multipart_upload_state(


I believe this is going to fail with simultaneous queries. For now I think it would be fine to error out in that case, but I'm not sure where to introduce the synchronization point to catch the situation.

robertbindar requested a review from KiterLuc July 28, 2022 13:37

KiterLuc reviewed Jul 29, 2022

View reviewed changes

robertbindar force-pushed the rbin/ch10668/global_order_serialization branch from 8f5c5f1 to 018d6b6 Compare August 2, 2022 12:48

KiterLuc reviewed Aug 24, 2022

View reviewed changes

robertbindar force-pushed the rbin/ch10668/global_order_serialization branch 2 times, most recently from 49cabd6 to ebf2a27 Compare August 25, 2022 10:47

robertbindar force-pushed the rbin/ch10668/global_order_serialization branch from 05afd5d to 607b9d4 Compare September 8, 2022 11:23

robertbindar force-pushed the rbin/ch10668/global_order_serialization branch from 45840e3 to ecc66e9 Compare September 19, 2022 08:58

robertbindar marked this pull request as ready for review September 20, 2022 13:06

ihnorton self-requested a review September 23, 2022 02:31

robertbindar added 19 commits September 30, 2022 14:54

basic code skeleton for serialized global order writes

4f12a05

serialize fields of GlobalWriteState

54cf7ca

deserialization of global write state

fab314e

global write state serialization

4aa7442

working round-trip - single submit, draft test case

64ef300

multiple submits, working version

5896b74

extend testcase for nullable and varsize attributes, fix bugs

1271bf4

Get extra coverage by switching all existing

a4840c0

global write tests to serializated roundtrip submits/finalize

fix uninitialized field in SingleCoord constructor

4d2cb3b

Fix bad initialization of nonempty domain for sparse during deseriali…

d29873c

…zation

tile alignment restrictions for remote global order writes

2ad5946

tile alignment restriction should be waived for submit_and_finalize r…

7889bb2

…oute

regular finalize() call should fail for remote global order writes

e5f43a8

rest client submit_and_finalize

31dc09a

also add cpp api for submit_and_finalize for easier testing

remove irelevant todo comments

ad0c092

capnp spec extension for multipart uploads

c839a8a

multipart upload support for global order writes - part1

e915548

multipart upload support for global order writes - part2

1db21c4

multipart upload support for global order writes - part3

baaf528

robertbindar and others added 27 commits September 30, 2022 14:56

Query submit should flush the s3 file buffers

af3f6bb

S3 bucket, key and status needs to be serialized as well. Code simplification. Getting s3 multipart state can be done with a single accessor function

Serialize has_delete_meta_ field as part of fragment_metadata

2f608ca

subarray boundary checking should happen after the subarray is deseri…

b870493

…alized

Serialization should clear S3 multipart uploads

a1af43c

When cloud executors die, context gets destructed, so S3::disconnect tries to complete multipart request

fix check_tile_alignment

8f457ba

return a client error if input buffers are <5mb as a temporary limita…

e4d09f6

…tion

enable serialized global order write tests

322adaa

serialization support for WrittenFragmentInfo, fix failing test

9d878e1

build failure when serialization is disabled

a56acaf

fix CI build failure

71c3194

fix wrong last tile boundary calculation in test case

c5c0e82

Fix bug in Query for sparse.

8676cf9

The coords_info_ markers need to be reset for remote global order writes too, otherwise the client fails a check for equal size input coords buffer between submits

CI should be green - part1

e7c3b64

Fix bug in unit-cppapi-var-offsets heap buffer overflow

c984e58

CI should be green - part2

aa4d062

flush_multipart_file_buffer should be noop for not supported backends

32f1f10

disable serialization wrappers for 32bit offsets gl.o.writes

0e84ee2

query_to_capnp doesn't handle well uint32 offset buffers, so tiledb_buffer_list_flatten ends up overflowing the offset buffers

Disable temporarily serial wrappers for 32-bit dense writes too

70bd8b6

fix test failure

f9f280e

Flushing of multipart buffers should only happen

8534fa7

for remote global order writes. Standalone S3 global order writes should still buffer as normal

win32 capnp headers

1d98417

ifdef serialization in unit-capi-dense_array

66b2aca

further fixes in unit-capi-dense_array

9c43ab6

revert incorrect change in unit-capi_dense_array

4254658

one more unit-capi_dense_array fix

0bf5bd9

add inline comments in the capnp global order writes structures

bdcb0b0

add V1 naming to writer globalWriteState capnp member

8d51519

robertbindar force-pushed the rbin/ch10668/global_order_serialization branch from 9eca90b to 8d51519 Compare September 30, 2022 12:01

ihnorton approved these changes Oct 4, 2022

View reviewed changes

ihnorton merged commit c26d1e4 into dev Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for remote global order writes #3393

Add support for remote global order writes #3393

robertbindar commented Jul 28, 2022

shortcut-integration bot commented Jul 28, 2022

KiterLuc Jul 29, 2022

KiterLuc left a comment

KiterLuc Aug 24, 2022

KiterLuc Aug 24, 2022

KiterLuc Aug 24, 2022

KiterLuc Aug 24, 2022

KiterLuc Aug 24, 2022

KiterLuc Aug 24, 2022

KiterLuc Aug 24, 2022

KiterLuc Aug 24, 2022

KiterLuc Aug 24, 2022

KiterLuc Aug 24, 2022

KiterLuc Aug 24, 2022

ihnorton left a comment

ihnorton Sep 28, 2022

ihnorton Sep 28, 2022

ihnorton Sep 28, 2022

ihnorton Sep 28, 2022

ihnorton Sep 28, 2022

ihnorton Sep 28, 2022

ihnorton Sep 28, 2022

		@@ -815,6 +823,141 @@ class FragmentMetadata {
		*/
		const shared_ptr<const ArraySchema>& array_schema() const;

Add support for remote global order writes #3393

Add support for remote global order writes #3393

Conversation

robertbindar commented Jul 28, 2022

shortcut-integration bot commented Jul 28, 2022

Choose a reason for hiding this comment

KiterLuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihnorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment