frankgh commented on code in PR #4661:
URL: https://github.com/apache/cassandra/pull/4661#discussion_r2967405718
##########
src/java/org/apache/cassandra/service/snapshot/TableSnapshot.java:
##########
@@ -43,6 +43,8 @@ public class TableSnapshot
private final String keyspaceName;
private final String tableName;
+ // tableId may be null under some rare circumstance namely pre-2.1 table
+ // whose snapshot is loaded upon startup rather than created while the jvm
is running
Review Comment:
tiny NIT: maybe annotate with Nullable
```suggestion
// whose snapshot is loaded upon startup rather than created while the
jvm is running
@Nullable
```
##########
src/java/org/apache/cassandra/service/snapshot/TableSnapshot.java:
##########
@@ -284,7 +288,7 @@ TableSnapshot build()
protected static String buildSnapshotId(String keyspaceName, String
tableName, UUID tableId, String tag)
{
- return String.format("%s:%s:%s:%s", keyspaceName, tableName, tableId,
tag);
+ return String.format("%s:%s:%s:%s", keyspaceName, tableName, tableId
== null ? "" : tableId, tag);
Review Comment:
minor NIT, annotate the `tableId` param with`@Nullable`
##########
src/java/org/apache/cassandra/service/snapshot/SnapshotLoader.java:
##########
@@ -149,7 +154,37 @@ private void loadSnapshotFromDir(Matcher
snapshotDirMatcher, Path snapshotDir)
{
String keyspaceName = snapshotDirMatcher.group("keyspace");
String tableName = snapshotDirMatcher.group("tableName");
- UUID tableId = parseUUID(snapshotDirMatcher.group("tableId"));
+ final UUID tableId;
Review Comment:
NIT: move to a helper method
```suggestion
UUID tableId = maybeDetermineTableId(snapshotDirMatcher,
snapshotDir, keyspaceName, tableName);
```
...
```
private UUID maybeDetermineTableId(Matcher snapshotDirMatcher, Path
snapshotDir, String keyspaceName, String tableName)
{
UUID tableId = null;
if (snapshotDirMatcher.group("tableId") == null)
{
logger.debug("Snapshot directory without tableId found
(pre-2.1 format): {}", snapshotDir);
// If we don't have a tableId in folder name (e.g pre 2.1
created table)
// Then attempt to get tableId from CFS on startup
// falling back to null is fine as it still yields a unique
result in buildSnapshotId for pre-2.1 table
if (Keyspace.isInitialized() &&
Schema.instance.getKeyspaceMetadata(keyspaceName) != null)
{
ColumnFamilyStore cfs =
ColumnFamilyStore.getIfExists(keyspaceName, tableName);
tableId = cfs != null && cfs.metadata.id != null
? cfs.metadata.id.asUUID()
: null;
if (tableId == null)
{
logger.warn("Snapshot directory without tableId
found (pre-2.1 format) " +
"unable to initialize tableId from cfs
defaulting to null {}", snapshotDir);
}
}
else
{
logger.warn("Snapshot directory without tableId found
(pre-2.1 format) " +
"Keyspace not initliazed or missing schema
unable to initialize tableId default to null {}", snapshotDir);
}
}
else
{
tableId = parseUUID(snapshotDirMatcher.group("tableId"));
}
return tableId;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
