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

Split timestamp_end_opened_at_ into 2 separate timestamps #3633

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

jp-dark
Copy link
Contributor

@jp-dark jp-dark commented Nov 4, 2022

The private Array variable timestamp_end_opened_at_ is being used for two different use cases:

  1. Specify end of timestamp range for fragments to load from file.
  2. Specify timestamp to use for new fragments, metadata, etc.

Split the private variable into two variables to make this explicit.


TYPE: IMPROVEMENT
DESC: Internal clean-up of array timestamps

@jp-dark jp-dark requested a review from KiterLuc November 4, 2022 15:36
@jp-dark jp-dark force-pushed the jpd/array-timestamp-fix-1 branch 2 times, most recently from 0da5d8a to 201d4fa Compare November 4, 2022 17:26
tiledb/sm/array/array.cc Outdated Show resolved Hide resolved
tiledb/sm/array/array.h Outdated Show resolved Hide resolved
tiledb/sm/array/array.cc Outdated Show resolved Hide resolved
tiledb/sm/array/array.h Outdated Show resolved Hide resolved
tiledb/sm/array/array.cc Outdated Show resolved Hide resolved
@jp-dark jp-dark requested a review from KiterLuc November 7, 2022 15:47
@jp-dark jp-dark requested a review from ihnorton November 8, 2022 15:22
The private `Array` variable `timestamp_end_opened_at_` is being used
for two different use cases:

1. Specify end of timestamp range for fragments to load from file.
2. Specify timestamp to use for new fragments, metadata, etc.

Split the private variable into two variables to make this explicit and
clean-up code that previously used timestamp_end_opened_at_.

* Also addresses feedback from @KiterLuc
@jp-dark jp-dark force-pushed the jpd/array-timestamp-fix-1 branch from 5047e09 to d8aeb72 Compare November 3, 2023 13:20
Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

To start with, this change looks good to me. It definitely makes a lot of the logic more obvious than it was previously.

I'm only marking this review as a comment as it's mostly a "I wonder if..." sort of review. Feel free to ignore it if you don't think its worth pursuing.

So, I wonder if this could be further improved by creating an ArrayDirectoryTimeRange class (or something with a better name). I'm basically thinking something like:

class ArrayDirectoryTimeRange {
  public:
    ArrayDirectoryTimeRange(optional<uint64_t> start, optional<uint64_t> end)
        : start_(start)
        , end_(end) {}

    std::pair<uint64_t, uint64_t> time_range_for_reads();
    uint64_t time_for_writes();
    
    /* whatever other helper APIs */

  private:
    optional<uint64_t> start_;
    optional<uint64_t> end_;
}

I haven't thought through all the APIs that might be needed, but from what I see there is one fundamental thing happening (The user provided start and/or end timestamps) and then we adjust how we use those based on the situation (i.e., reads vs writes vs reopen vs other things).

Also, if you're not immediately convinced, I'm already on the hook for improving the UPDATE and DELETE handling in ArrayDirectory next sprint so can probably dedicate a couple hours to giving this a whirl. It seems like one of those things where it'll be pretty clear right away whether its going to be better or worse.

Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

Also, an official +1 in case that wasn't obvious.

@johnkerl johnkerl changed the title Splits timestamp_end_opened_at_ into 2 separate timestamps Split timestamp_end_opened_at_ into 2 separate timestamps Nov 3, 2023
@jp-dark
Copy link
Contributor Author

jp-dark commented Nov 3, 2023

So, I wonder if this could be further improved by creating an ArrayDirectoryTimeRange class

I think that would be a reasonable follow-up task. This PR was always intended to be a first step in improving our handling of timestamps.

@davisp
Copy link
Contributor

davisp commented Nov 3, 2023

More than reasonable!

@KiterLuc
Copy link
Contributor

KiterLuc commented Nov 4, 2023

@KiterLuc KiterLuc merged commit 78cd951 into dev Nov 4, 2023
55 checks passed
@KiterLuc KiterLuc deleted the jpd/array-timestamp-fix-1 branch November 4, 2023 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants