SFS ADRs based on Design Review Session 2023-05-05

[irq0]

Signed-off-by: Marcel Lauhoff marcel.lauhoff@suse.com

[giubacc]

I've found what the null version is: https://docs.aws.amazon.com/AmazonS3/latest/userguide/AddingObjectstoVersionSuspendedBuckets.html

Basically the null version occurs whenever a versioned bucket becomes unversioned because versioning has been suspended.
In this case, if you PUT an object, its version becomes the null version, that is, a fake id denoting the null version (that can be overwritten by subsequent PUTs).
If you delete an object on a versioned bucket a it is needed to check if the null version exists and in case it will be deleted.
So the null version is the transposition of the "regular" object of an unversioned bucket in the world of the versioned buckets.
I think it could be worth to add this information to the ADR.

[jecluis]

maybe move this to docs/design ? Makes more sense I think.

[jecluis]

LGTM, but I'd like the doc to be moved to docs/design/ instead. That, or we'll need to bring the other doc there here. I do think there's an advantage in keeping design docs all in the same place though.

[irq0]

LGTM, but I'd like the doc to be moved to docs/design/ instead. That, or we'll need to bring the other doc there here. I do think there's an advantage in keeping design docs all in the same place though.

I don't have a strong opinion on either one directory or two.
Only thing I would love to have is a README that tells us what goes where. I can't intuitively distinguish between a "architecture decision record" and a design document.

So, what goes where?

doc/decisions

• how we work (0000-use-markdown-any-decision-records.md, 0001-reorganize-documentation.md, 0004-release-captain.md, 0005-project-labels.md, 0006-git-branches-tags.md, 0007-release-process.md, 0008-pull-requests-code-review-guide.md)

doc/design

• how we want the software to work (0002-s3gw-storage-backend.md, 0003-sfs.md, 0009-sfs-collection-of-high-level-design-decisions.md, 0010-sfs-versioning.md)

I think doc/decisions is too confusing. Suggest the following:

• doc/decisions/design - how we want the software to work
• doc/decisions/how-we-work - how we work docs from above

[jecluis]

* how we work (0000-use-markdown-any-decision-records.md, 0001-reorganize-documentation.md, 0004-release-captain.md, 0005-project-labels.md, 0006-git-branches-tags.md, 0007-release-process.md, 0008-pull-requests-code-review-guide.md)

doc/design

* how we want the software to work (0002-s3gw-storage-backend.md, 0003-sfs.md, 0009-sfs-collection-of-high-level-design-decisions.md, 0010-sfs-versioning.md)

I think doc/decisions is too confusing. Suggest the following:

* doc/decisions/design - how we want the software to work

* doc/decisions/how-we-work - how we work docs from above

That division sounds good to me. We might want to renumber them though, per subdir? @jhmarina thoughts?

[jecluis]

@irq0 in the meantime, given that's a tangential topic, lets just go ahead and merge this. That can be handled on a different PR.

[jecluis]

@irq0 in the meantime, given that's a tangential topic, lets just go ahead and merge this. That can be handled on a different PR.

And I mean "merge once everybody has reviewed and is happy (or not too sad)" :)

[0xavi0]

The current implementation uses de id of the version. The greater id is the latest version. (id is the primary key of the versioned object table)
If we want to implement this based on time... Wouldn't it be better to use modification time instead to creation time?. In case that 2 threads are putting a version in parallel, using the creation time means the last trying to write wins, while using the modification time means the last that finished writing wins. I think it makes more sense that the one winning is the last writing?

Also, if we want to query what's the latest version based on creation/modification time we might need to change the schema, because right now those time columns are stored as ceph::real_time which is stored as a BLOB in the database.

Problem is that maybe the SQLite data structures have not enough precision?

From the SQLite documentation

SQLite does not have a storage class set aside for storing dates and/or times. Instead, the built-in [Date And Time Functions](https://www.sqlite.org/lang_datefunc.html) of SQLite are capable of storing dates and times as TEXT, REAL, or INTEGER values:

TEXT as ISO8601 strings ("YYYY-MM-DD HH:MM:SS.SSS").
REAL as Julian day numbers, the number of days since noon in Greenwich on November 24, 4714 B.C. according to the proleptic Gregorian calendar.
INTEGER as Unix Time, the number of seconds since 1970-01-01 00:00:00 UTC.

[jecluis]

Also, if we want to query what's the latest version based on creation/modification time we might need to change the schema, because right now those time columns are stored as ceph::real_time which is stored as a BLOB in the database.

On the multipart work I'm using timestamps to store a ceph::real_time instead, which I think is stored as a double. The conversion is trivial to do, and we get to use those fields. I didn't think about the necessary precision though, I should double check that.

[irq0]

Looks like we need to standardize on a time format and add that to the high level doc.

For reference:
ceph::real_time is std::chrono::time_point<real_clock>
real_clock is Ceph's high resolution real time clock. Nanosecond resolution.

I'd love to keep as much resolution as we can. Minimum microseconds. At the same time leveraging sqlite indices, range queries, sorting is important too.

Options:

1. Store as ISO8601 strings. We loose resolution. Values are monotonically increasing. order by and range queries work nicely. We can use sqlite datetime functions.
2. Store as integer. Unix time is second resolution = meh. So store micos? nanos? SQLite has max 64bit integers. I don't thing struct timespec fits good into this.
3. Store as multiple columns. ISO8601 string + nanoseconds part or two integers. First for search, second for resolution.
4. Store as blob. I read somewhere[42] that sqlite does comparisons based on memcmp on this. If we encode the time data right, we can have it comparable and monotonically increasing. No idea if we can do BETWEEN queries and how it indexes. Probably the least desirable format.

What else?

[42] https://www.sqlitetutorial.net/sqlite-data-types/

[0xavi0]

IMO, we should try to keep the same precision that rgw has... (nanos).
I would prefer not to have things in more than 1 column.

Isn't 2 ^ 64 enough? .... it should cover up to year 2554, right?
I think SQLite stores signed INTEGERS, though....

[jecluis]

I'm using ceph::real_clock::to_double(mp.mtime) to obtain a double, which I assumed would capture a high resolution clock value, but now I'm not so sure. It seems to translate to

std::chrono::duration<double>(t.time_since_epoch()).count();

which seems to be using the default for std::chrono::duration, that being "seconds" as far as I can tell.

[irq0]

And finally... something I wanted to investigate long ago when all this started and it's custom type binding in sqlite_orm

Very cool! Wanted to try the binding support for a while as well :). Looks good. It could replace the conversion layer we have if we add bindings for the ceph types, right?

[0xavi0]

This would move the conversion to another place, but the conversion code will be still needed.
We won't see any performance gain. (and the code to be maintained will be highly templated code, which not everybody loves)

This is something I wanted to investigate since the very beginning but given that we didn't had the time I could not go deeper.

I think it makes user code cleaner, because we work with our own types.

Anyway, I would maybe do it for the timestamps in this iteration and will try to change the rest when the versioning stuff is done. I don't think this is as urgent as versioning work.

[0xavi0]

Test code for storing ceph::real_clock::time_point as int64 and checking that values stored or read are not negative (throws a std::runtime_error in that case)

I can order_by and use max and min from the sqlite storage class same as with the 16 chars hex string representation.

Trying to pass a time_point like ceph::real_clock::time_point(std::chrono::nanoseconds(std::numeric_limits<uint64_t>::max())) won't be accepted.

Error converting ceph::real_clock::time_point to int64. Nanoseconds value: 18446744073709551615 is out of range

namespace ceph {
static int64_t to_int64(const ceph::real_clock::time_point& t) {
    uint64_t nanos = std::chrono::duration_cast<std::chrono::nanoseconds>(t.time_since_epoch()).count();
    // we check that the value is not greater than int64 max
    if (nanos > std::numeric_limits<int64_t>::max()) {
      std::stringstream oss;
      oss << "Error converting ceph::real_clock::time_point to int64. Nanoseconds value: " << nanos << " is out of range";
      throw std::runtime_error(oss.str());
    }
    return static_cast<int64_t>(nanos);
}

static std::optional<ceph::real_clock::time_point> from_int64(int64_t int_time) {
    std::optional<ceph::real_clock::time_point> ret;
    if (int_time < 0) {
      // to ensure that we stick to the int64 positive range.
      std::stringstream oss;
      oss << "Error converting int64 nanoseconds value to ceph::real_cock::time_point. Value: " << int_time << " is out of range";
      throw std::runtime_error(oss.str());
    }
    uint64_t uint64_nanos = static_cast<uint64_t>(int_time);
    ceph::real_clock::time_point ret_time = ceph::real_clock::time_point(std::chrono::nanoseconds(uint64_nanos));
    ret = ret_time;
    return ret;
}
} // namespace ceph

namespace sqlite_orm
{
  template<>
  struct type_printer<ceph::real_clock::time_point> : public integer_printer {};

  template<>
  struct statement_binder<ceph::real_clock::time_point> {

      int bind(sqlite3_stmt* stmt, int index, const ceph::real_clock::time_point& value) const {
          return statement_binder<uint64_t>().bind(stmt, index, to_int64(value));
      }
  };

  template<>
  struct field_printer<ceph::real_clock::time_point> {
      std::string operator()(const ceph::real_clock::time_point& t) const {
          auto int_value = ceph::to_int64(t);
          return std::to_string(int_value);
      }
  };

  template<>
  struct row_extractor<ceph::real_clock::time_point> {
      ceph::real_clock::time_point extract(int64_t row_value) const {
          if(row_value < 0) {
             throw std::runtime_error("incorrect encoded timestamp int64. (" + std::to_string(row_value) + ") is out of range");
          }
          auto sd = ceph::from_int64(row_value);
          if(sd) {
            return sd.value();
          } else {
            throw std::runtime_error("incorrect encoded timestamp int64 (" + std::to_string(row_value) + ")");
          }
      }

      ceph::real_clock::time_point extract(sqlite3_stmt* stmt, int columnIndex) const {
        std::cout << "Extracting value" << std::endl;
        auto int_value = sqlite3_column_int64(stmt, columnIndex);
        return this->extract(int_value);
      }
  };
}

[0xavi0]

As I know there is a lot of noise I'm making a summary:

Options I see so far.

• Store timestamps as int64 (which will limit our timestamps range up to year 2262)
• Store timestamps as 16 chars hex string (we can store the whole uint64 range and, if in the future upstream adds more precision we can increase the number of chars... This comes with a conversion code that adds latency)
• Store as uint64 (but we cannot query by timestamps because sqlite takes sign into account for order_by and max, main)
• Keep storing it as a blob if we don't need to query by timestamp.

And... for deciding how to identify what's the last version of an object.

• Max creation time
• Max modification time (or another field that we create so the last thread writing wins)
• Max version id (I think we can go for this in favor of max creation time, because they are equivalents and we don't need to change the timestamps code. When we set creation time we also create a new version id, so the greater one will still be the latest one)

[irq0]

I'd like to add option int64, but with microsecond resolution (max 2554-07-21 23:34:33.709551).

[irq0]

Last push adds:
Timestamp discussion as adr 11
ADR 10: Timestamps to record, refinement of the last version rule.

[irq0]

Now has a bit about the set_mtime / mtime

[0xavi0]

lgtm