Skip to content

Stronger type checking and other WAL improvements - #4082

Merged
ctubbsii merged 4 commits into
apache:mainfrom
ctubbsii:log-related-cleanup
Dec 19, 2023
Merged

Stronger type checking and other WAL improvements#4082
ctubbsii merged 4 commits into
apache:mainfrom
ctubbsii:log-related-cleanup

Conversation

@ctubbsii

Copy link
Copy Markdown
Member
  • Use LogEntry type in more places where just the logEntry's filename as a String was being used
  • Keep the UUID and HostAndPort in LogEntry that are created for validation, rather than their String equivalents
  • Remove no longer needed deleteWal(String) and create a deleteFromMutation method on LogEntry to keep the schema serialization/deserialization in LogEntry (may want to move that to LogColumnFamily class in the MetadataSchema later, but for now, at least it's in one place)
  • Use private constructor in LogEntry, do validation in a fromFilePath static method before calling the constructor with the strongly typed results
  • Update javadocs to clarify the filename format
  • Fix the validation of the host+port in the filename (it's a plus sign not a colon, because colons weren't valid characters in HDFS, so we used a plus instead)
  • Simplify VolumeUtil switchVolumes logic that was previously complicated because an old LogEntry format could contain multiple paths per entry and that's no longer the case
  • Clean up DfsLogger a little bit to remove some old stuff that wasn't needed: remove ServerResources used for testing when a mock ServerContext would suffice, pass LogEntry instead of Strings for the filename and its metadata format (the meta reference being passed wasn't actually what was in the table anyway, it was coming from LogEntry, which was computing it, and Ample wasn't using this meta for deletions/cleanup anyway - may be an existing bug; if so, that will be a follow-on), stop using toString() to compute serialization and remove the redundant use of the logger name itself to try to compute the serialization in that toString() method

* Use LogEntry type in more places where just the logEntry's filename as
  a String was being used
* Keep the UUID and HostAndPort in LogEntry that are created for
  validation, rather than their String equivalents
* Remove no longer needed deleteWal(String) and create a
  deleteFromMutation method on LogEntry to keep the schema
  serialization/deserialization in LogEntry (may want to move that to
  LogColumnFamily class in the MetadataSchema later, but for now, at
  least it's in one place)
* Use private constructor in LogEntry, do validation in a fromFilePath
  static method before calling the constructor with the strongly typed
  results
* Update javadocs to clarify the filename format
* Fix the validation of the host+port in the filename (it's a plus sign
  not a colon, because colons weren't valid characters in HDFS, so we
  used a plus instead)
* Simplify VolumeUtil switchVolumes logic that was previously
  complicated because an old LogEntry format could contain multiple
  paths per entry and that's no longer the case
* Clean up DfsLogger a little bit to remove some old stuff that wasn't
  needed: remove ServerResources used for testing when a mock
  ServerContext would suffice, pass LogEntry instead of Strings for the
  filename and its metadata format (the meta reference being passed
  wasn't actually what was in the table anyway, it was coming from
  LogEntry, which was computing it, and Ample wasn't using this meta
  for deletions/cleanup anyway - may be an existing bug; if so, that
  will be a follow-on), stop using toString() to compute serialization
  and remove the redundant use of the logger name itself to try to
  compute the serialization in that toString() method
@ctubbsii ctubbsii self-assigned this Dec 15, 2023

@DomGarguilo DomGarguilo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Haven't reviewed everything yet but what I have reviewed looks good so far.

Comment thread core/src/test/java/org/apache/accumulo/core/tabletserver/log/LogEntryTest.java Outdated
Comment thread core/src/test/java/org/apache/accumulo/core/tabletserver/log/LogEntryTest.java Outdated
Comment thread core/src/main/java/org/apache/accumulo/core/tabletserver/log/LogEntry.java Outdated

@EdColeman EdColeman left a comment

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.

LGTM. The may be somethings that can be looked as a follow-on, but this really cleans things up and helps future changes stand-out and not get lost in a big PR.

@keith-turner keith-turner left a comment

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.

This is a nice cleanup.

Comment thread core/src/test/java/org/apache/accumulo/core/tabletserver/log/LogEntryTest.java Outdated
Comment thread core/src/main/java/org/apache/accumulo/core/tabletserver/log/LogEntry.java Outdated
* Simplify names to use "path" instead of "filePath" or "filename"
* Remove unneeded String.toString
* Avoid use of Java NIO Path type in LogEntryTest, which is confusing
  since the paths actually come from HDFS Path
* Add check to verify `+` exists for port delimiter
@ctubbsii

Copy link
Copy Markdown
Member Author

I think I addressed all the comments. I have a follow-up to this in PR #4085 that also addresses issue #4061, and created issue #4086 as another follow-on, that is only loosely related.

Comment on lines +70 to +77
String badTServerMsg =
"Invalid tserver in path. Expected: host+port. Found '" + tserverPart + "'";
if (tserverPart.contains(":") || !tserverPart.contains("+")) {
throw new IllegalArgumentException(badTServerMsg);
}
HostAndPort tserver;
try {
HostAndPort.fromString(tserverPart);
tserver = HostAndPort.fromString(tserverPart.replace("+", ":"));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess this is expected behavior but it seems odd that the following code causes the highlighed exception to be thrown since the host and port are separated by a : rather than +:

      HostAndPort server = HostAndPort.fromParts("server1", 8555); // server1:8555

      String walFilePath =
          java.nio.file.Path.of(server.toString(), UUID.randomUUID().toString()).toString();
      LogEntry wal = LogEntry.fromPath(walFilePath);

Especially since internally, the tserver string is being converted to a HostAndPost object. Not suggesting that any changes need to be or can be made, just thought it was interesting.

@ctubbsii ctubbsii Dec 21, 2023

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.

You can't use server.toString(), because it produces a string with a colon in it, and that's not valid for HDFS.

When we write this to a file, we replace the colons with a plus sign, and vice versa. That may not have been obvious in your tests because you used java.io.file.Path, rather than Hadoop's Path type.

If you don't convert the plus sign to a colon, HostAndPort will still parse it, but it will put everything in the host portion, and leave the port empty. So:

HostAndPort.fromString("localhost+8080"); // results in {host=localhost+8080, port=}
HostAndPort.fromString("localhost:8080"); // results in {host=localhost, port=8080}

It's hard to tell, because HostAndPort.toString() looks the same as what you'd expect if it had treated the plus sign as a delimiter, so toString() returns what you put in, even though it's not valid.

When we write the file to disk, we actually convert it to use the plus sign:

String logger = Joiner.on("+").join(address.split(":"));

@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.

4 participants