[Kernel] Add Snapshot::getTimestamp public API
#3791
Conversation
| // Internal APIs // | ||
| /////////////////// | ||
|
|
||
| public Path getLogPath() { |
There was a problem hiding this comment.
- put all getters at the top of internal APIs, with more complicated internal APIs below
- re-ordered these getters to match the order of member field declarations
scottsand-db
requested review from
allisonport-db and
vkorukanti
and removed request for
allisonport-db
| public Metadata getMetadata() { | ||
| return metadata; | ||
| /////////////////// | ||
| // Internal APIs // |
There was a problem hiding this comment.
This comment seems misleading. Even if this line says "Internal", 3rd party developers may call the subsequent method as they are public.
| @@ -36,6 +36,21 @@ public interface Snapshot { | |||
| */ | |||
| long getVersion(Engine engine); | |||
|
|
|||
| /** | |||
| * Get the timestamp (in milliseconds since the Unix epoch) of the latest commit of this snapshot. | |||
| * For an uninitialized snapshot, this returns -1. | |||
There was a problem hiding this comment.
Can you specify what "uninitialized snapshot" means?
There was a problem hiding this comment.
This was just met copying + pasting the existing javadocs.
Let me think about this. We should probably just say "for an empty table"
| @Override | ||
| public long getVersion(Engine engine) { | ||
| return version; | ||
| } | ||
|
|
||
| @Override | ||
| public long getTimestamp(Engine engine) { | ||
| if (TableConfig.isICTEnabled(engine, metadata)) { |
There was a problem hiding this comment.
I'm wondering if maybe this should be part of the snapshot creation (when we also load protocol & metadata)?
There was a problem hiding this comment.
getTimestamp also kind of implies that we don't do any real work to retrieve this
There was a problem hiding this comment.
I'm wondering if maybe this should be part of the snapshot creation (when we also load protocol & metadata)?
That's a great point! Let me re-think this ...
There was a problem hiding this comment.
This will be a decent amount of work to do this. I wish this had been done with ICT was implemented in Kernel.
I do think that loading this during our P&M log replay is most correct. We would include the commit info in the read schema. And we would rename this from "P&M" log replay to something else..
@vkorukanti any thoughts here?
There was a problem hiding this comment.
I just spent several hours implementing this, and realized that CommitInfo isn't stored in the checkpoint ... so, loading ICT from logReplay would only work when the latest log segment is a delta file not a checkpoint. darn.
Which Delta project/connector is this regarding?
Description
Add a new
Snapshot::getTimestamppublic API.How was this patch tested?
N/A. Trivial.
Does this PR introduce any user-facing changes?
Yes. Adds a new
Snapshot::getTimestamppublic API.