Skip to content

Use LogEntry concrete type in more places - #4085

Merged
ctubbsii merged 1 commit into
apache:mainfrom
ctubbsii:log-related-cleanup2
Dec 21, 2023
Merged

Use LogEntry concrete type in more places#4085
ctubbsii merged 1 commit into
apache:mainfrom
ctubbsii:log-related-cleanup2

Conversation

@ctubbsii

Copy link
Copy Markdown
Member
  • Track the actual column qualifier read from the metadata table, so when it is deleted, it can delete the actual entry, rather than try to delete the existing entry with the new format (requires no longer strictly requiring that log entries start with a -/). This fixes Ensure log entries in the metadata table are up-to-date on upgrade (or remove strict validation) #4061 for the main branch for 3.1; a similar issue may still exist for 2.1, but will require a different fix.
  • Remove use of Collection<Collection<String>> and use Collection<LogEntry> where possible, since the former was for a previous WAL storage format that we do not support, and the latter preserves the strongly-typed LogEntry in more places

* Track the actual column qualifier read from the metadata table, so
  when it is deleted, it can delete the actual entry, rather than try to
  delete the existing entry with the new format (requires no longer
  strictly requiring that log entries start with a `-/`).
  This fixes apache#4061 for the main branch for 3.1; a similar issue may
  still exist for 2.1, but will require a different fix.
* Remove use of `Collection<Collection<String>>` and use
  `Collection<LogEntry>` where possible, since the former was for a
  previous WAL storage format that we do not support, and the latter
  preserves the strongly-typed LogEntry in more places

public boolean recoverLogs(KeyExtent extent, Collection<Collection<String>> walogs)
throws IOException {
public boolean recoverLogs(KeyExtent extent, Collection<LogEntry> walogs) throws IOException {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Protip: this diff looks bigger than it is in GitHub. Best to ignore whitespace / indentation while comparing for a more friendly view.

return fromPath(parts[1]);
Text qualifier = entry.getKey().getColumnQualifier();
String[] parts = qualifier.toString().split("/", 2);
Preconditions.checkArgument(parts.length == 2, "Malformed write-ahead log %s", qualifier);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you verify the contents of parts[0] here? Not sure if that is possible when considering possible old code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, the parts[0] could contain - (current code) or tserver:port (previous version). I can't be sure about prior to that, but I believe there was an upgrade step that removed relative paths from WAL entries that would have rewritten anything that would be older than these two most recent versions.

My concern about adding strict validation on the ignored part is that we could cause an unnecessary recovery failure. Accommodating variance in the part that doesn't matter by not validating its content avoids that.

I just have a lot of uncertainty about the history of the various formats of these columns. I would prefer not to strictly validate the ignored part, and we just make sure we go through upgrade code for any future metadata schema changes.

@ctubbsii
ctubbsii merged commit 37117b2 into apache:main Dec 21, 2023
@ctubbsii
ctubbsii deleted the log-related-cleanup2 branch December 21, 2023 05:15
@ctubbsii ctubbsii added this to the 3.1.0 milestone Jul 12, 2024
@ctubbsii ctubbsii modified the milestones: 3.1.0, 4.0.0 Mar 14, 2025
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.

Ensure log entries in the metadata table are up-to-date on upgrade (or remove strict validation)

3 participants