adelapena commented on code in PR #2916:
URL: https://github.com/apache/cassandra/pull/2916#discussion_r1400701236
##########
src/java/org/apache/cassandra/index/sai/disk/format/IndexDescriptor.java:
##########
@@ -267,10 +266,7 @@ public FileHandle
createPerSSTableFileHandle(IndexComponent indexComponent, Thro
final File file = fileFor(indexComponent);
if (logger.isTraceEnabled())
- {
- logger.trace(logMessage("Opening {} file handle for {} ({})"),
- file,
FBUtilities.prettyPrintMemory(file.length()));
- }
+ logger.trace(logMessage("Opening {} file handle for {} ({})"),
file, FBUtilities.prettyPrintMemory(file.length()));
Review Comment:
There are three `{}` placeholders for only two arguments.
##########
src/java/org/apache/cassandra/index/sai/disk/format/IndexDescriptor.java:
##########
@@ -347,36 +345,36 @@ public long sizeOnDiskOfPerSSTableComponents()
.sum();
}
- public long sizeOnDiskOfPerIndexComponents(IndexContext indexContext)
+ public long sizeOnDiskOfPerIndexComponents(IndexTermType indexTermType,
IndexIdentifier indexIdentifier)
{
return version.onDiskFormat()
- .perColumnIndexComponents(indexContext)
+ .perColumnIndexComponents(indexTermType)
.stream()
- .map(c -> fileFor(c, indexContext))
+ .map(c -> fileFor(c, indexIdentifier))
.filter(File::exists)
.mapToLong(File::length)
.sum();
}
@VisibleForTesting
- public long sizeOnDiskOfPerIndexComponent(IndexComponent indexComponent,
IndexContext indexContext)
+ public long sizeOnDiskOfPerIndexComponent(IndexComponent indexComponent,
IndexIdentifier indexIdentifier)
{
- File componentFile = fileFor(indexComponent, indexContext);
+ File componentFile = fileFor(indexComponent, indexIdentifier);
return componentFile.exists() ? componentFile.length() : 0;
}
@SuppressWarnings("BooleanMethodIsAlwaysInverted")
- public boolean validatePerIndexComponents(IndexContext indexContext,
IndexValidation validation)
+ public boolean validatePerIndexComponents(IndexTermType indexTermType,
IndexIdentifier indexIdentifier, IndexValidation validation)
{
if (validation == IndexValidation.NONE)
return true;
- logger.info(indexContext.logMessage("Validating per-column index
components using mode " + validation));
+ logger.info(logMessage("Validating per-column index components for " +
indexIdentifier + " using mode " + validation));
Review Comment:
```suggestion
logger.info(logMessage("Validating per-column index components for
{} using mode {}"), indexIdentifier, validation);
```
##########
src/java/org/apache/cassandra/index/sai/disk/format/OnDiskFormat.java:
##########
@@ -42,11 +43,11 @@
* <ul>
* <li>Methods taking no parameters. These methods return static
information about the
* format. This can include static information about the per-sstable
components</li>
- * <li>Methods taking just an {@link IndexContext}. These methods return
static information
+ * <li>Methods taking just a {@link StorageAttachedIndex}. These methods
return static information
Review Comment:
I think there aren't such methods.
##########
src/java/org/apache/cassandra/index/sai/disk/v1/SSTableIndexWriter.java:
##########
@@ -62,11 +62,13 @@ public class SSTableIndexWriter implements
PerColumnIndexWriter
public static final long MAX_STRING_TERM_SIZE =
SAI_MAX_STRING_TERM_SIZE.getSizeInBytes();
public static final long MAX_FROZEN_TERM_SIZE =
SAI_MAX_FROZEN_TERM_SIZE.getSizeInBytes();
public static final long MAX_VECTOR_TERM_SIZE =
SAI_MAX_VECTOR_TERM_SIZE.getSizeInBytes();
- public static final String TERM_OVERSIZE_MESSAGE = "Can't add term of
column {} to index for key: {}, term size {} " +
+ public static final String TERM_OVERSIZE_MESSAGE = "Can't add term of
column {} to index for: {}, term size {} " +
"max allowed size {},
use analyzed = true (if not yet set) for that column.";
private final IndexDescriptor indexDescriptor;
- private final IndexContext indexContext;
+ private final StorageAttachedIndex index;
+ private final IndexTermType indexTermType;
+ private final IndexIdentifier indexIdentifier;
Review Comment:
Maybe we should choose between storing a reference to the
`StorageAttachedIndex` and storing references to its relevant components
(`IndexTermType`, `IndexIdentifier`, `AbstractAnalyzer`, `IndexMetrics` and
`IndexWriterConfig`). Having references to both the container and its contents
might feel redundant, wdyt?
##########
src/java/org/apache/cassandra/index/sai/disk/v1/SSTableIndexWriter.java:
##########
@@ -166,20 +175,20 @@ public void abort(Throwable cause)
{
aborted = true;
- logger.warn(indexContext.logMessage("Aborting SSTable index flush for
{}..."), indexDescriptor.sstableDescriptor, cause);
+ logger.warn(indexIdentifier.logMessage("Aborting SSTable index flush
for {}..."), indexDescriptor.sstableDescriptor, cause);
// It's possible for the current builder to be unassigned after we
flush a final segment.
if (currentBuilder != null)
{
// If an exception is thrown out of any writer operation prior to
successful segment
// flush, we will end up here, and we need to free up builder
memory tracked by the limiter:
long allocated = currentBuilder.totalBytesAllocated();
- long globalBytesUsed = currentBuilder.release(indexContext);
- logger.debug(indexContext.logMessage("Aborting index writer for
SSTable {} released {}. Global segment memory usage now at {}."),
+ long globalBytesUsed = currentBuilder.release(indexIdentifier);
+ logger.debug(indexIdentifier.logMessage("Aborting index writer for
SSTable {} released {}. Global segment memory usage now at {}."),
indexDescriptor.sstableDescriptor,
FBUtilities.prettyPrintMemory(allocated),
FBUtilities.prettyPrintMemory(globalBytesUsed));
}
- indexDescriptor.deleteColumnIndex(indexContext);
+ indexDescriptor.deleteColumnIndex(index.termType(), indexIdentifier);
Review Comment:
```suggestion
indexDescriptor.deleteColumnIndex(indexTermType, indexIdentifier);
```
##########
src/java/org/apache/cassandra/index/sai/disk/format/Version.java:
##########
@@ -127,24 +127,24 @@ public FileNameFormatter fileNameFormatter()
public interface FileNameFormatter
{
- String format(IndexComponent indexComponent, IndexContext
indexContext);
+ String format(IndexComponent indexComponent, IndexIdentifier
indexIdentifier);
}
/**
* SAI default filename formatter. This is the current SAI on-disk
filename format
- *
+ * <p>
* Format: {@code <sstable descriptor>-SAI+<version>(+<index
name>)+<component name>.db}
* Note: The index name is excluded for per-SSTable index files that are
shared
* across all the per-column indexes for the SSTable.
*/
- private static String defaultFileNameFormat(IndexComponent indexComponent,
IndexContext indexContext, String version)
+ private static String defaultFileNameFormat(IndexComponent indexComponent,
IndexIdentifier indexIdentifier, String version)
Review Comment:
Can we mark the `indexIdentifier` parameter as `@Nullable`?
--
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]
