From be9ee23dd39d353ee9b12cadfddc92a9f02a1d54 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Thu, 10 Apr 2025 15:10:03 +0530 Subject: [PATCH 01/30] Initial commit --- data-loader/build.gradle | 1 + .../dataimport/log/AbstractImportLogger.java | 170 +++++++++++ .../dataimport/log/ImportLoggerConfig.java | 13 + .../dataimport/log/LogStorageLocation.java | 7 + .../log/SingleFileImportLogger.java | 140 +++++++++ .../log/SplitByDataChunkImportLogger.java | 185 ++++++++++++ .../dataimport/log/writer/AwsS3LogWriter.java | 30 ++ .../log/writer/DefaultLogWriterFactory.java | 36 +++ .../log/writer/LocalFileLogWriter.java | 64 +++++ .../dataimport/log/writer/LogFileType.java | 8 + .../core/dataimport/log/writer/LogWriter.java | 15 + .../log/writer/LogWriterFactory.java | 8 + .../log/writer/LogWriterFactoryConfig.java | 15 + .../log/SingleFileImportLoggerTest.java | 271 ++++++++++++++++++ .../log/SplitByDataChunkImportLoggerTest.java | 244 ++++++++++++++++ .../writer/DefaultLogWriterFactoryTest.java | 67 +++++ 16 files changed, 1274 insertions(+) create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/LogStorageLocation.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogFileType.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactory.java create mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactoryConfig.java create mode 100644 data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java create mode 100644 data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java create mode 100644 data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java diff --git a/data-loader/build.gradle b/data-loader/build.gradle index 87a057933b..5e9c2a4ba0 100644 --- a/data-loader/build.gradle +++ b/data-loader/build.gradle @@ -17,6 +17,7 @@ subprojects { implementation("org.apache.commons:commons-lang3:${commonsLangVersion}") implementation("commons-io:commons-io:${commonsIoVersion}") implementation("org.slf4j:slf4j-simple:${slf4jVersion}") + implementation("software.amazon.awssdk:s3:${awssdkVersion}") // Mockito testImplementation "org.mockito:mockito-core:${mockitoVersion}" diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java new file mode 100644 index 0000000000..eac36c802c --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java @@ -0,0 +1,170 @@ +package com.scalar.db.dataloader.core.dataimport.log; + +import com.fasterxml.jackson.databind.JsonNode; +import com.scalar.db.dataloader.core.Constants; +import com.scalar.db.dataloader.core.DataLoaderObjectMapper; +import com.scalar.db.dataloader.core.dataimport.ImportEventListener; +import com.scalar.db.dataloader.core.dataimport.datachunk.ImportDataChunkStatus; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriter; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactory; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTargetResult; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTargetResultStatus; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; +import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; +import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchStatus; +import lombok.RequiredArgsConstructor; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +@RequiredArgsConstructor +public abstract class AbstractImportLogger implements ImportEventListener { + + protected static final DataLoaderObjectMapper OBJECT_MAPPER = new DataLoaderObjectMapper(); + + protected final ImportLoggerConfig config; + protected final LogWriterFactory logWriterFactory; + protected final List listeners = new ArrayList<>(); + + public void addListener(ImportEventListener listener) { + listeners.add(listener); + } + + public void removeListener(ImportEventListener listener) { + listeners.remove(listener); + } + + @Override + public void onDataChunkStarted(ImportDataChunkStatus importDataChunkStatus) { + // Currently we are not logging the start of a data chunk + } + + @Override + public void onTransactionBatchStarted(ImportTransactionBatchStatus batchStatus) { + // Currently we are not logging the start of a transaction batch + notifyTransactionBatchStarted(batchStatus); + } + + @Override + public void onTransactionBatchCompleted(ImportTransactionBatchResult batchResult) { + // skip logging success records if the configuration is set to skip + if (shouldSkipLoggingSuccess(batchResult)) { + return; + } + + logTransactionBatch(batchResult); + notifyTransactionBatchCompleted(batchResult); + } + + @Override + public void onTaskComplete(ImportTaskResult taskResult) { + // TODO: we can remove this event if it's current not being used in the import Manager as well + } + + protected abstract void logTransactionBatch(ImportTransactionBatchResult batchResult); + + protected boolean shouldSkipLoggingSuccess(ImportTransactionBatchResult batchResult) { + return batchResult.isSuccess() && !config.isLogSuccessRecords(); + } + + protected JsonNode createFilteredTransactionBatchLogJsonNode( + ImportTransactionBatchResult batchResult) { + + // If the batch result does not contain any records, return the batch result as is + if (batchResult.getRecords() == null) { + return OBJECT_MAPPER.valueToTree(batchResult); + } + + // Create a new list to store the modified import task results + List modifiedRecords = new ArrayList<>(); + + // Loop over the records in the batchResult + for (ImportTaskResult taskResult : batchResult.getRecords()) { + // Create a new ImportTaskResult and not add the raw record yet + List targetResults = + batchResult.isSuccess() + ? taskResult.getTargets() + : updateTargetStatusForAbortedTransactionBatch(taskResult.getTargets()); + ImportTaskResult.ImportTaskResultBuilder builder = + ImportTaskResult.builder() + .rowNumber(taskResult.getRowNumber()) + .targets(targetResults) + .dataChunkId(taskResult.getDataChunkId()) + .rowNumber(taskResult.getRowNumber()); + + // Only add the raw record if the configuration is set to log raw source data + if (config.isLogRawSourceRecords()) { + builder.rawRecord(taskResult.getRawRecord()); + } + ImportTaskResult modifiedTaskResult = builder.build(); + + // Add the modified task result to the list + modifiedRecords.add(modifiedTaskResult); + } + + // Create a new transaction batch result with the modified import task results + ImportTransactionBatchResult modifiedBatchResult = + ImportTransactionBatchResult.builder() + .dataChunkId(batchResult.getDataChunkId()) + .transactionBatchId(batchResult.getTransactionBatchId()) + .transactionId(batchResult.getTransactionId()) + .records(modifiedRecords) + .errors(batchResult.getErrors()) + .success(batchResult.isSuccess()) + .build(); + + // Convert the modified batch result to a JsonNode + return OBJECT_MAPPER.valueToTree(modifiedBatchResult); + } + + protected void closeLogWriter(LogWriter logWriter) { + if (logWriter != null) { + try { + logWriter.close(); + } catch (IOException e) { + logError("Failed to close a log writer", e); + } + } + } + + protected abstract void logError(String errorMessage, Exception e); + + protected LogWriter createLogWriter(String logFilePath) throws IOException { + return logWriterFactory.createLogWriter(logFilePath); + } + + private void notifyTransactionBatchStarted(ImportTransactionBatchStatus status) { + for (ImportEventListener listener : listeners) { + listener.onTransactionBatchStarted(status); + } + } + + private void notifyTransactionBatchCompleted(ImportTransactionBatchResult batchResult) { + for (ImportEventListener listener : listeners) { + listener.onTransactionBatchCompleted(batchResult); + } + } + + private List updateTargetStatusForAbortedTransactionBatch( + List targetResults) { + for (int i = 0; i < targetResults.size(); i++) { + ImportTargetResult target = targetResults.get(i); + if (target.getStatus().equals(ImportTargetResultStatus.SAVED)) { + ImportTargetResult newTarget = + ImportTargetResult.builder() + .importAction(target.getImportAction()) + .status(ImportTargetResultStatus.ABORTED) + .importedRecord(target.getImportedRecord()) + .namespace(target.getNamespace()) + .tableName(target.getTableName()) + .dataMapped(target.isDataMapped()) + .errors(Collections.singletonList(Constants.ABORT_TRANSACTION_STATUS)) + .build(); + targetResults.set(i, newTarget); + } + } + return targetResults; + } +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java new file mode 100644 index 0000000000..fc0039bf90 --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java @@ -0,0 +1,13 @@ +package com.scalar.db.dataloader.core.dataimport.log; + +import lombok.Builder; +import lombok.Value; + +@Value +@Builder +public class ImportLoggerConfig { + String logDirectoryPath; + boolean logSuccessRecords; + boolean logRawSourceRecords; + boolean prettyPrint; +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/LogStorageLocation.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/LogStorageLocation.java new file mode 100644 index 0000000000..396cb3d8e4 --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/LogStorageLocation.java @@ -0,0 +1,7 @@ +package com.scalar.db.dataloader.core.dataimport.log; + +/** The location where the logs are stored. */ +public enum LogStorageLocation { + LOCAL_FILE_STORAGE, + AWS_S3 +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java new file mode 100644 index 0000000000..e851631468 --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java @@ -0,0 +1,140 @@ +package com.scalar.db.dataloader.core.dataimport.log; + +import com.fasterxml.jackson.databind.JsonNode; +import com.scalar.db.dataloader.core.dataimport.datachunk.ImportDataChunkStatus; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriter; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactory; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTargetResult; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTargetResultStatus; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; +import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; + +public class SingleFileImportLogger extends AbstractImportLogger { + + protected static final String SUMMARY_LOG_FILE_NAME = "summary.log"; + protected static final String SUCCESS_LOG_FILE_NAME = "success.json"; + protected static final String FAILURE_LOG_FILE_NAME = "failure.json"; + private static final Logger LOGGER = LoggerFactory.getLogger(SingleFileImportLogger.class); + private LogWriter summaryLogWriter; + private LogWriter successLogWriter; + private LogWriter failureLogWriter; + + public SingleFileImportLogger(ImportLoggerConfig config, LogWriterFactory logWriterFactory) + throws IOException { + super(config, logWriterFactory); + successLogWriter = createLogWriter(config.getLogDirectoryPath() + SUCCESS_LOG_FILE_NAME); + failureLogWriter = createLogWriter(config.getLogDirectoryPath() + FAILURE_LOG_FILE_NAME); + } + + @Override + public void onTaskComplete(ImportTaskResult taskResult) { + if (!config.isLogSuccessRecords() && !config.isLogRawSourceRecords()) return; + try { + writeImportTaskResultDetailToLogs(taskResult); + } catch (Exception e) { + logError("Failed to write success/failure logs", e); + } + } + + @Override + public void addOrUpdateDataChunkStatus(ImportDataChunkStatus status) {} + + @Override + public void onDataChunkCompleted(ImportDataChunkStatus dataChunkStatus) { + try { + logDataChunkSummary(dataChunkStatus); + } catch (IOException e) { + logError("Failed to log the data chunk summary", e); + } + } + + @Override + public void onAllDataChunksCompleted() { + closeAllLogWriters(); + } + + @Override + protected void logTransactionBatch(ImportTransactionBatchResult batchResult) { + try { + LogWriter logWriter = getLogWriterForTransactionBatch(batchResult); + JsonNode jsonNode = createFilteredTransactionBatchLogJsonNode(batchResult); + writeToLogWriter(logWriter, jsonNode); + } catch (IOException e) { + logError("Failed to write a transaction batch record to the log file", e); + } + } + + @Override + protected void logError(String errorMessage, Exception exception) { + LOGGER.error(errorMessage, exception); + } + + private void logDataChunkSummary(ImportDataChunkStatus dataChunkStatus) throws IOException { + if (summaryLogWriter == null) { + summaryLogWriter = createLogWriter(config.getLogDirectoryPath() + SUMMARY_LOG_FILE_NAME); + } + writeImportDataChunkSummary(dataChunkStatus, summaryLogWriter); + } + + private void writeImportDataChunkSummary( + ImportDataChunkStatus dataChunkStatus, LogWriter logWriter) throws IOException { + JsonNode jsonNode = OBJECT_MAPPER.valueToTree(dataChunkStatus); + writeToLogWriter(logWriter, jsonNode); + } + + private LogWriter getLogWriterForTransactionBatch(ImportTransactionBatchResult batchResult) + throws IOException { + String logFileName = batchResult.isSuccess() ? SUCCESS_LOG_FILE_NAME : FAILURE_LOG_FILE_NAME; + LogWriter logWriter = batchResult.isSuccess() ? successLogWriter : failureLogWriter; + if (logWriter == null) { + logWriter = createLogWriter(config.getLogDirectoryPath() + logFileName); + if (batchResult.isSuccess()) { + successLogWriter = logWriter; + } else { + failureLogWriter = logWriter; + } + } + return logWriter; + } + + private void writeImportTaskResultDetailToLogs(ImportTaskResult importTaskResult) + throws IOException { + JsonNode jsonNode; + for (ImportTargetResult target : importTaskResult.getTargets()) { + if (config.isLogSuccessRecords() + && target.getStatus().equals(ImportTargetResultStatus.SAVED)) { + synchronized (successLogWriter) { + jsonNode = OBJECT_MAPPER.valueToTree(target); + successLogWriter.write(jsonNode); + successLogWriter.flush(); + } + } + if (config.isLogRawSourceRecords() + && !target.getStatus().equals(ImportTargetResultStatus.SAVED)) { + synchronized (failureLogWriter) { + jsonNode = OBJECT_MAPPER.valueToTree(target); + failureLogWriter.write(jsonNode); + failureLogWriter.flush(); + } + } + } + } + + private void writeToLogWriter(LogWriter logWriter, JsonNode jsonNode) throws IOException { + logWriter.write(jsonNode); + logWriter.flush(); + } + + private void closeAllLogWriters() { + closeLogWriter(summaryLogWriter); + closeLogWriter(successLogWriter); + closeLogWriter(failureLogWriter); + summaryLogWriter = null; + successLogWriter = null; + failureLogWriter = null; + } +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java new file mode 100644 index 0000000000..f12e6e5f73 --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java @@ -0,0 +1,185 @@ +package com.scalar.db.dataloader.core.dataimport.log; + +import com.fasterxml.jackson.databind.JsonNode; +import com.scalar.db.dataloader.core.dataimport.datachunk.ImportDataChunkStatus; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogFileType; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriter; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactory; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTargetResult; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTargetResultStatus; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; +import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; + +public class SplitByDataChunkImportLogger extends AbstractImportLogger { + + protected static final String SUMMARY_LOG_FILE_NAME_FORMAT = "data_chunk_%s_summary.json"; + protected static final String FAILURE_LOG_FILE_NAME_FORMAT = "data_chunk_%s_failure.json"; + protected static final String SUCCESS_LOG_FILE_NAME_FORMAT = "data_chunk_%s_success.json"; + + private static final Logger LOGGER = LoggerFactory.getLogger(SplitByDataChunkImportLogger.class); + private final Map summaryLogWriters = new HashMap<>(); + private final Map successLogWriters = new HashMap<>(); + private final Map failureLogWriters = new HashMap<>(); + + public SplitByDataChunkImportLogger( + ImportLoggerConfig config, LogWriterFactory logWriterFactory) { + super(config, logWriterFactory); + } + + @Override + public void onTaskComplete(ImportTaskResult taskResult) { + if (!config.isLogSuccessRecords() && !config.isLogRawSourceRecords()) return; + try { + writeImportTaskResultDetailToLogs(taskResult); + } catch (IOException e) { + LOGGER.error("Failed to write success/failure logs"); + } + } + + private void writeImportTaskResultDetailToLogs(ImportTaskResult importTaskResult) + throws IOException { + JsonNode jsonNode; + for (ImportTargetResult target : importTaskResult.getTargets()) { + if (config.isLogSuccessRecords() + && target.getStatus().equals(ImportTargetResultStatus.SAVED)) { + jsonNode = OBJECT_MAPPER.valueToTree(target); + synchronized (successLogWriters) { + LogWriter successLogWriter = + initializeLogWriterIfNeeded(LogFileType.SUCCESS, importTaskResult.getDataChunkId()); + successLogWriter.write(jsonNode); + successLogWriter.flush(); + } + } + if (config.isLogRawSourceRecords() + && !target.getStatus().equals(ImportTargetResultStatus.SAVED)) { + jsonNode = OBJECT_MAPPER.valueToTree(target); + synchronized (failureLogWriters) { + LogWriter failureLogWriter = + initializeLogWriterIfNeeded(LogFileType.FAILURE, importTaskResult.getDataChunkId()); + failureLogWriter.write(jsonNode); + failureLogWriter.flush(); + } + } + } + } + + @Override + public void addOrUpdateDataChunkStatus(ImportDataChunkStatus status) {} + + @Override + public void onDataChunkCompleted(ImportDataChunkStatus dataChunkStatus) { + try { + logDataChunkSummary(dataChunkStatus); + // Close the split log writers per data chunk if they exist for this data chunk id + closeLogWritersForDataChunk(dataChunkStatus.getDataChunkId()); + } catch (IOException e) { + LOGGER.error("Failed to log the data chunk summary", e); + } + } + + @Override + public void onAllDataChunksCompleted() { + closeAllDataChunkLogWriters(); + } + + @Override + protected void logTransactionBatch(ImportTransactionBatchResult batchResult) { + LogFileType logFileType = batchResult.isSuccess() ? LogFileType.SUCCESS : LogFileType.FAILURE; + try (LogWriter logWriter = + initializeLogWriterIfNeeded(logFileType, batchResult.getDataChunkId())) { + JsonNode jsonNode = createFilteredTransactionBatchLogJsonNode(batchResult); + synchronized (logWriter) { + logWriter.write(jsonNode); + logWriter.flush(); + } + } catch (IOException e) { + LOGGER.error("Failed to write a transaction batch record to a split mode log file", e); + } + } + + @Override + protected void logError(String errorMessage, Exception exception) { + LOGGER.error(errorMessage, exception); + } + + private void logDataChunkSummary(ImportDataChunkStatus dataChunkStatus) throws IOException { + try (LogWriter logWriter = + initializeLogWriterIfNeeded(LogFileType.SUMMARY, dataChunkStatus.getDataChunkId())) { + logWriter.write(OBJECT_MAPPER.valueToTree(dataChunkStatus)); + logWriter.flush(); + } + } + + private void closeLogWritersForDataChunk(int dataChunkId) { + closeLogWriter(successLogWriters.remove(dataChunkId)); + closeLogWriter(failureLogWriters.remove(dataChunkId)); + closeLogWriter(summaryLogWriters.remove(dataChunkId)); + } + + private void closeAllDataChunkLogWriters() { + summaryLogWriters.values().forEach(this::closeLogWriter); + successLogWriters.values().forEach(this::closeLogWriter); + failureLogWriters.values().forEach(this::closeLogWriter); + summaryLogWriters.clear(); + successLogWriters.clear(); + failureLogWriters.clear(); + } + + private String getLogFilePath(long batchId, LogFileType logFileType) { + String logfilePath; + switch (logFileType) { + case SUCCESS: + logfilePath = + config.getLogDirectoryPath() + String.format(SUCCESS_LOG_FILE_NAME_FORMAT, batchId); + break; + case FAILURE: + logfilePath = + config.getLogDirectoryPath() + String.format(FAILURE_LOG_FILE_NAME_FORMAT, batchId); + break; + case SUMMARY: + logfilePath = + config.getLogDirectoryPath() + String.format(SUMMARY_LOG_FILE_NAME_FORMAT, batchId); + break; + default: + logfilePath = ""; + } + return logfilePath; + } + + private LogWriter initializeLogWriterIfNeeded(LogFileType logFileType, int dataChunkId) + throws IOException { + Map logWriters = getLogWriters(logFileType); + if (!logWriters.containsKey(dataChunkId)) { + LogWriter logWriter = createLogWriter(logFileType, dataChunkId); + logWriters.put(dataChunkId, logWriter); + } + return logWriters.get(dataChunkId); + } + + private LogWriter createLogWriter(LogFileType logFileType, int dataChunkId) throws IOException { + String logFilePath = getLogFilePath(dataChunkId, logFileType); + return createLogWriter(logFilePath); + } + + private Map getLogWriters(LogFileType logFileType) { + Map logWriterMap = null; + switch (logFileType) { + case SUCCESS: + logWriterMap = successLogWriters; + break; + case FAILURE: + logWriterMap = failureLogWriters; + break; + case SUMMARY: + logWriterMap = summaryLogWriters; + break; + } + return logWriterMap; + } +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java new file mode 100644 index 0000000000..857fc47a69 --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java @@ -0,0 +1,30 @@ +package com.scalar.db.dataloader.core.dataimport.log.writer; + +import com.fasterxml.jackson.databind.JsonNode; +import lombok.AllArgsConstructor; +import software.amazon.awssdk.services.s3.S3AsyncClient; + +import java.io.IOException; + +@AllArgsConstructor +public class AwsS3LogWriter implements LogWriter { + + private final S3AsyncClient s3AsyncClient; + private final String bucketName; + private final String objectKey; + + @Override + public void write(JsonNode sourceRecord) throws IOException { + // Implementation to write content to cloud storage + } + + @Override + public void flush() throws IOException { + // Implementation to flush content to cloud storage + } + + @Override + public void close() throws IOException { + // Implementation to close the cloud storage connection + } +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java new file mode 100644 index 0000000000..6940f8d16a --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java @@ -0,0 +1,36 @@ +package com.scalar.db.dataloader.core.dataimport.log.writer; + +import com.scalar.db.dataloader.core.dataimport.log.ImportLoggerConfig; +import lombok.AllArgsConstructor; + +import java.io.IOException; + +/** A factory class to create log writers. */ +@AllArgsConstructor +public class DefaultLogWriterFactory implements LogWriterFactory { + + private final LogWriterFactoryConfig config; + private final ImportLoggerConfig importLoggerConfig; + + /** + * Creates a log writer based on the configuration. + * + * @param logFilePath the path of the log file + * @return the log writer + */ + @Override + public LogWriter createLogWriter(String logFilePath) throws IOException { + LogWriter logWriter = null; + switch (config.getLogStorageLocation()) { + case LOCAL_FILE_STORAGE: + logWriter = new LocalFileLogWriter(logFilePath, importLoggerConfig); + break; + case AWS_S3: + logWriter = + new AwsS3LogWriter( + config.getS3AsyncClient(), config.getBucketName(), config.getObjectKey()); + break; + } + return logWriter; + } +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java new file mode 100644 index 0000000000..eb152b6e1d --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java @@ -0,0 +1,64 @@ +package com.scalar.db.dataloader.core.dataimport.log.writer; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.databind.JsonNode; +import com.scalar.db.dataloader.core.DataLoaderObjectMapper; +import com.scalar.db.dataloader.core.dataimport.log.ImportLoggerConfig; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.StandardOpenOption; + +public class LocalFileLogWriter implements LogWriter { + private final JsonGenerator logWriter; + private final DataLoaderObjectMapper objectMapper; + + /** + * Creates an instance of LocalFileLogWriter with the specified file path and log file type. + * + * @param filePath the file path + * @throws IOException if an I/O error occurs + */ + public LocalFileLogWriter(String filePath, ImportLoggerConfig importLoggerConfig) + throws IOException { + Path path = Paths.get(filePath); + this.objectMapper = new DataLoaderObjectMapper(); + this.logWriter = + objectMapper + .getFactory() + .createGenerator( + Files.newBufferedWriter( + path, StandardOpenOption.CREATE, StandardOpenOption.APPEND)); + // Start the JSON array + if (importLoggerConfig.isPrettyPrint()) this.logWriter.useDefaultPrettyPrinter(); + this.logWriter.writeStartArray(); + this.logWriter.flush(); + } + + @Override + public void write(JsonNode sourceRecord) throws IOException { + if (sourceRecord == null) { + return; + } + synchronized (logWriter) { + objectMapper.writeValue(logWriter, sourceRecord); + } + } + + @Override + public void flush() throws IOException { + logWriter.flush(); + } + + @Override + public void close() throws IOException { + if (logWriter.isClosed()) { + return; + } + logWriter.writeEndArray(); + logWriter.flush(); + logWriter.close(); + } +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogFileType.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogFileType.java new file mode 100644 index 0000000000..5483aefc91 --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogFileType.java @@ -0,0 +1,8 @@ +package com.scalar.db.dataloader.core.dataimport.log.writer; + +/** The type of the log writer. */ +public enum LogFileType { + SUCCESS, + FAILURE, + SUMMARY +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java new file mode 100644 index 0000000000..cfd713acc3 --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java @@ -0,0 +1,15 @@ +package com.scalar.db.dataloader.core.dataimport.log.writer; + +import com.fasterxml.jackson.databind.JsonNode; + +import java.io.IOException; + +public interface LogWriter extends AutoCloseable { + + void write(JsonNode sourceRecord) throws IOException; + + void flush() throws IOException; + + @Override + void close() throws IOException; +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactory.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactory.java new file mode 100644 index 0000000000..b3c4dfc080 --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactory.java @@ -0,0 +1,8 @@ +package com.scalar.db.dataloader.core.dataimport.log.writer; + +import java.io.IOException; + +public interface LogWriterFactory { + + LogWriter createLogWriter(String logFilePath) throws IOException; +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactoryConfig.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactoryConfig.java new file mode 100644 index 0000000000..901d0aae6f --- /dev/null +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactoryConfig.java @@ -0,0 +1,15 @@ +package com.scalar.db.dataloader.core.dataimport.log.writer; + +import com.scalar.db.dataloader.core.dataimport.log.LogStorageLocation; +import lombok.Builder; +import lombok.Value; +import software.amazon.awssdk.services.s3.S3AsyncClient; + +@Builder +@Value +public class LogWriterFactoryConfig { + LogStorageLocation logStorageLocation; + S3AsyncClient s3AsyncClient; + String bucketName; + String objectKey; +} diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java new file mode 100644 index 0000000000..cd80cf61d1 --- /dev/null +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java @@ -0,0 +1,271 @@ +package com.scalar.db.dataloader.core.dataimport.log; + +import com.fasterxml.jackson.core.type.TypeReference; +import com.scalar.db.dataloader.core.DataLoaderObjectMapper; +import com.scalar.db.dataloader.core.dataimport.datachunk.ImportDataChunkStatus; +import com.scalar.db.dataloader.core.dataimport.log.writer.DefaultLogWriterFactory; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactory; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactoryConfig; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; +import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class SingleFileImportLoggerTest { + + private static final Logger LOGGER = LoggerFactory.getLogger(SingleFileImportLoggerTest.class); + private static final DataLoaderObjectMapper OBJECT_MAPPER = new DataLoaderObjectMapper(); + + @TempDir Path tempDir; + + private LogWriterFactory logWriterFactory; + + @BeforeEach + void setUp() { + LogWriterFactoryConfig logWriterFactoryConfig = + LogWriterFactoryConfig.builder() + .logStorageLocation(LogStorageLocation.LOCAL_FILE_STORAGE) + .build(); + ImportLoggerConfig importLoggerConfig = + ImportLoggerConfig.builder() + .prettyPrint(false) + .logSuccessRecords(false) + .logRawSourceRecords(false) + .logDirectoryPath("path") + .build(); + logWriterFactory = new DefaultLogWriterFactory(logWriterFactoryConfig, importLoggerConfig); + } + + @AfterEach + void tearDown() throws IOException { + cleanUpTempDir(); + } + + private void cleanUpTempDir() throws IOException { + try (Stream paths = Files.list(tempDir)) { + paths.forEach(this::deleteFile); + } + } + + private void deleteFile(Path file) { + try { + Files.deleteIfExists(file); + } catch (IOException e) { + LOGGER.error("Failed to delete file: {}", file, e); + } + } + + @Test + void onTransactionBatchCompleted_NoErrors_ShouldWriteToSuccessLogFile() throws IOException { + testTransactionBatchCompleted(true, true); + } + + @Test + void onTransactionBatchCompleted_HasErrors_ShouldWriteToFailureLogFile() throws IOException { + testTransactionBatchCompleted(false, true); + } + + private void testTransactionBatchCompleted(boolean success, boolean logSuccessRecords) + throws IOException { + // Arrange + ImportLoggerConfig config = + ImportLoggerConfig.builder() + .logDirectoryPath(tempDir.toString() + "/") + .logRawSourceRecords(true) + .logSuccessRecords(logSuccessRecords) + .build(); + SingleFileImportLogger importLogger = new SingleFileImportLogger(config, logWriterFactory); + + List batchResults = createBatchResults(1, success); + + // Act + for (ImportTransactionBatchResult batchResult : batchResults) { + importLogger.onTransactionBatchCompleted(batchResult); + importLogger.onDataChunkCompleted( + ImportDataChunkStatus.builder().dataChunkId(batchResult.getDataChunkId()).build()); + } + importLogger.onAllDataChunksCompleted(); + + // Assert + assertTransactionBatchResults(batchResults, success, logSuccessRecords); + } + + private List createBatchResults(int count, boolean success) { + List batchResults = new ArrayList<>(); + + for (int i = 1; i <= count; i++) { + List records = + Collections.singletonList( + ImportTaskResult.builder() + .rowNumber(i) + .rawRecord(OBJECT_MAPPER.createObjectNode()) + .targets(Collections.EMPTY_LIST) + .build()); + ImportTransactionBatchResult result = + ImportTransactionBatchResult.builder() + .dataChunkId(i) + .transactionBatchId(1) + .records(records) + .success(success) + .build(); + batchResults.add(result); + } + + return batchResults; + } + + private void assertTransactionBatchResults( + List batchResults, boolean success, boolean logSuccessRecords) + throws IOException { + DataLoaderObjectMapper objectMapper = new DataLoaderObjectMapper(); + + // Single file log mode + Path logFileName = + tempDir.resolve( + success + ? SingleFileImportLogger.SUCCESS_LOG_FILE_NAME + : SingleFileImportLogger.FAILURE_LOG_FILE_NAME); + if (logSuccessRecords || !success) { + assertTrue(Files.exists(logFileName), "Log file should exist"); + + String logContent = new String(Files.readAllBytes(logFileName), StandardCharsets.UTF_8); + + List logEntries = + objectMapper.readValue( + logContent, new TypeReference>() {}); + + assertEquals( + batchResults.size(), + logEntries.size(), + "Number of log entries should match the number of batch results"); + + for (int i = 0; i < batchResults.size(); i++) { + assertTransactionBatchResult(batchResults.get(i), logEntries.get(i)); + } + } else { + assertFalse(Files.exists(logFileName), "Log file should not exist"); + } + } + + private void assertTransactionBatchResult( + ImportTransactionBatchResult expected, ImportTransactionBatchResult actual) { + assertEquals(expected.getDataChunkId(), actual.getDataChunkId(), "Data chunk ID should match"); + assertEquals( + expected.getTransactionBatchId(), + actual.getTransactionBatchId(), + "Transaction batch ID should match"); + assertEquals( + expected.getTransactionId(), actual.getTransactionId(), "Transaction ID should match"); + assertEquals(expected.isSuccess(), actual.isSuccess(), "Success status should match"); + + List expectedRecords = expected.getRecords(); + List actualRecords = actual.getRecords(); + assertEquals(expectedRecords.size(), actualRecords.size(), "Number of records should match"); + for (int j = 0; j < expectedRecords.size(); j++) { + ImportTaskResult expectedRecord = expectedRecords.get(j); + ImportTaskResult actualRecord = actualRecords.get(j); + assertEquals( + expectedRecord.getRowNumber(), actualRecord.getRowNumber(), "Row number should match"); + assertEquals( + expectedRecord.getRawRecord(), actualRecord.getRawRecord(), "Raw record should match"); + assertEquals(expectedRecord.getTargets(), actualRecord.getTargets(), "Targets should match"); + } + } + + @Test + void onDataChunkCompleted_NoErrors_ShouldWriteToSummaryLogFile() throws IOException { + testDataChunkCompleted(false); + } + + @Test + void onDataChunkCompleted_HasErrors_ShouldWriteToSummaryLogFile() throws IOException { + testDataChunkCompleted(true); + } + + private void testDataChunkCompleted(boolean hasErrors) throws IOException { + ImportLoggerConfig config = + ImportLoggerConfig.builder() + .logDirectoryPath(tempDir.toString() + "/") + .logRawSourceRecords(true) + .logSuccessRecords(true) + .build(); + SingleFileImportLogger importLogger = new SingleFileImportLogger(config, logWriterFactory); + + List dataChunkStatuses = + Stream.of(1, 2) + .map(id -> createDataChunkStatus(id, hasErrors)) + .collect(Collectors.toList()); + + dataChunkStatuses.forEach(importLogger::onDataChunkCompleted); + importLogger.onAllDataChunksCompleted(); + + assertDataChunkStatusLog(SingleFileImportLogger.SUMMARY_LOG_FILE_NAME, dataChunkStatuses); + } + + private ImportDataChunkStatus createDataChunkStatus(int dataChunkId, boolean hasErrors) { + return ImportDataChunkStatus.builder() + .dataChunkId(dataChunkId) + .startTime(Instant.now()) + .endTime(Instant.now()) + .totalRecords(100) + .successCount(hasErrors ? 90 : 100) + .failureCount(hasErrors ? 10 : 0) + .batchCount(5) + .totalDurationInMilliSeconds(1000) + .build(); + } + + private void assertDataChunkStatusLog( + String logFilePattern, List dataChunkStatuses) throws IOException { + assertSingleFileLog(tempDir, logFilePattern, dataChunkStatuses); + } + + private void assertSingleFileLog( + Path tempDir, String logFileName, List dataChunkStatuses) + throws IOException { + Path summaryLogFile = tempDir.resolve(logFileName); + assertTrue(Files.exists(summaryLogFile)); + + String logContent = new String(Files.readAllBytes(summaryLogFile), StandardCharsets.UTF_8); + DataLoaderObjectMapper objectMapper = new DataLoaderObjectMapper(); + List logEntries = + objectMapper.readValue(logContent, new TypeReference>() {}); + + assertEquals(dataChunkStatuses.size(), logEntries.size()); + for (int i = 0; i < dataChunkStatuses.size(); i++) { + assertDataChunkStatusEquals(dataChunkStatuses.get(i), logEntries.get(i)); + } + } + + private void assertDataChunkStatusEquals( + ImportDataChunkStatus expected, ImportDataChunkStatus actual) { + assertEquals(expected.getDataChunkId(), actual.getDataChunkId()); + assertEquals(expected.getStartTime(), actual.getStartTime()); + assertEquals(expected.getEndTime(), actual.getEndTime()); + assertEquals(expected.getTotalRecords(), actual.getTotalRecords()); + assertEquals(expected.getSuccessCount(), actual.getSuccessCount()); + assertEquals(expected.getFailureCount(), actual.getFailureCount()); + assertEquals(expected.getBatchCount(), actual.getBatchCount()); + assertEquals( + expected.getTotalDurationInMilliSeconds(), actual.getTotalDurationInMilliSeconds()); + } +} diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java new file mode 100644 index 0000000000..e1f397caf5 --- /dev/null +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java @@ -0,0 +1,244 @@ +package com.scalar.db.dataloader.core.dataimport.log; + +import com.fasterxml.jackson.core.type.TypeReference; +import com.scalar.db.dataloader.core.DataLoaderObjectMapper; +import com.scalar.db.dataloader.core.dataimport.datachunk.ImportDataChunkStatus; +import com.scalar.db.dataloader.core.dataimport.log.writer.DefaultLogWriterFactory; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactory; +import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactoryConfig; +import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; +import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.IntStream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class SplitByDataChunkImportLoggerTest { + + private static final DataLoaderObjectMapper OBJECT_MAPPER = new DataLoaderObjectMapper(); + + @TempDir Path tempDir; + + private LogWriterFactory logWriterFactory; + + @BeforeEach + void setUp() { + LogWriterFactoryConfig logWriterFactoryConfig = + LogWriterFactoryConfig.builder() + .logStorageLocation(LogStorageLocation.LOCAL_FILE_STORAGE) + .build(); + ImportLoggerConfig importLoggerConfig = + ImportLoggerConfig.builder() + .prettyPrint(false) + .logSuccessRecords(false) + .logRawSourceRecords(false) + .logDirectoryPath("path") + .build(); + logWriterFactory = new DefaultLogWriterFactory(logWriterFactoryConfig, importLoggerConfig); + } + + @Test + void onTransactionBatchCompleted_NoErrors_ShouldWriteToDataChunkSuccessFiles() + throws IOException { + testTransactionBatchCompleted(true, true); + } + + @Test + void onTransactionBatchCompleted_HasErrors_ShouldWriteToDataChunkFailureFiles() + throws IOException { + testTransactionBatchCompleted(false, true); + } + + @Test + void onTransactionBatchCompleted_NoErrorsAndNoSuccessFileLogging_ShouldNotWriteToSuccessFiles() + throws IOException { + testTransactionBatchCompleted(true, false); + } + + private void testTransactionBatchCompleted(boolean success, boolean logSuccessRecords) + throws IOException { + // Arrange + ImportLoggerConfig config = + ImportLoggerConfig.builder() + .logDirectoryPath(tempDir.toString() + "/") + .logRawSourceRecords(true) + .logSuccessRecords(logSuccessRecords) + .build(); + SplitByDataChunkImportLogger importLogger = + new SplitByDataChunkImportLogger(config, logWriterFactory); + + List batchResults = new ArrayList<>(); + + for (int i = 1; i <= 3; i++) { + List records = + Collections.singletonList( + ImportTaskResult.builder() + .rowNumber(i) + .targets(Collections.EMPTY_LIST) + .rawRecord(OBJECT_MAPPER.createObjectNode()) + .build()); + ImportTransactionBatchResult result = + ImportTransactionBatchResult.builder() + .dataChunkId(i) + .transactionBatchId(1) + .records(records) + .success(success) + .build(); + batchResults.add(result); + } + + // Act + for (ImportTransactionBatchResult batchResult : batchResults) { + importLogger.onTransactionBatchCompleted(batchResult); + importLogger.onDataChunkCompleted( + ImportDataChunkStatus.builder().dataChunkId(batchResult.getDataChunkId()).build()); + } + importLogger.onAllDataChunksCompleted(); + + // Assert + for (int i = 0; i < batchResults.size(); i++) { + ImportTransactionBatchResult batchResult = batchResults.get(i); + String logFileNameFormat = + success + ? SplitByDataChunkImportLogger.SUCCESS_LOG_FILE_NAME_FORMAT + : SplitByDataChunkImportLogger.FAILURE_LOG_FILE_NAME_FORMAT; + Path dataChunkLogFileName = tempDir.resolve(String.format(logFileNameFormat, i + 1)); + + if (success && logSuccessRecords) { + assertTrue(Files.exists(dataChunkLogFileName), "Data chunk success log file should exist"); + assertTransactionBatchResult(batchResult, dataChunkLogFileName); + } else if (!success) { + assertTrue(Files.exists(dataChunkLogFileName), "Data chunk failure log file should exist"); + assertTransactionBatchResult(batchResult, dataChunkLogFileName); + } else { + assertFalse( + Files.exists(dataChunkLogFileName), "Data chunk success log file should not exist"); + } + } + } + + private void assertTransactionBatchResult( + ImportTransactionBatchResult expected, Path dataChunkLogFileName) throws IOException { + // String logContent = Files.readString(dataChunkLogFileName); + String logContent = + new String(Files.readAllBytes(dataChunkLogFileName), StandardCharsets.UTF_8); + DataLoaderObjectMapper objectMapper = new DataLoaderObjectMapper(); + List logEntries = + objectMapper.readValue( + logContent, new TypeReference>() {}); + ImportTransactionBatchResult actual = logEntries.get(0); + + assertEquals(expected.getDataChunkId(), actual.getDataChunkId(), "Data chunk ID should match"); + assertEquals( + expected.getTransactionBatchId(), + actual.getTransactionBatchId(), + "Transaction batch ID should match"); + assertEquals( + expected.getTransactionId(), actual.getTransactionId(), "Transaction ID should match"); + assertEquals(expected.isSuccess(), actual.isSuccess(), "Success status should match"); + + List expectedRecords = expected.getRecords(); + List actualRecords = actual.getRecords(); + assertEquals(expectedRecords.size(), actualRecords.size(), "Number of records should match"); + for (int j = 0; j < expectedRecords.size(); j++) { + ImportTaskResult expectedRecord = expectedRecords.get(j); + ImportTaskResult actualRecord = actualRecords.get(j); + assertEquals( + expectedRecord.getRowNumber(), actualRecord.getRowNumber(), "Row number should match"); + assertEquals( + expectedRecord.getRawRecord(), actualRecord.getRawRecord(), "Raw record should match"); + assertEquals(expectedRecord.getTargets(), actualRecord.getTargets(), "Targets should match"); + } + } + + @Test + void onDataChunkCompleted_NoErrors_ShouldWriteToSummaryLogFile() throws IOException { + testDataChunkCompleted( + String.format(SplitByDataChunkImportLogger.SUMMARY_LOG_FILE_NAME_FORMAT, "%d"), false); + } + + @Test + void onDataChunkCompleted_HasErrors_ShouldWriteToSummaryLogFile() throws IOException { + testDataChunkCompleted( + String.format(SplitByDataChunkImportLogger.SUMMARY_LOG_FILE_NAME_FORMAT, "%d"), true); + } + + private void testDataChunkCompleted(String logFilePattern, boolean hasErrors) throws IOException { + ImportLoggerConfig config = + ImportLoggerConfig.builder() + .logDirectoryPath(tempDir.toString() + "/") + .logRawSourceRecords(true) + .logSuccessRecords(true) + .build(); + SplitByDataChunkImportLogger importLogger = + new SplitByDataChunkImportLogger(config, logWriterFactory); + + List dataChunkStatuses = + IntStream.rangeClosed(1, 2) + .mapToObj(id -> createDataChunkStatus(id, hasErrors)) + .collect(Collectors.toList()); + + dataChunkStatuses.forEach(importLogger::onDataChunkCompleted); + importLogger.onAllDataChunksCompleted(); + + assertDataChunkStatusLog(logFilePattern, dataChunkStatuses); + } + + private ImportDataChunkStatus createDataChunkStatus(int dataChunkId, boolean hasErrors) { + return ImportDataChunkStatus.builder() + .dataChunkId(dataChunkId) + .startTime(Instant.now()) + .endTime(Instant.now()) + .totalRecords(100) + .successCount(hasErrors ? 90 : 100) + .failureCount(hasErrors ? 10 : 0) + .batchCount(5) + .totalDurationInMilliSeconds(1000) + .build(); + } + + private void assertDataChunkStatusLog( + String logFilePattern, List dataChunkStatuses) throws IOException { + for (ImportDataChunkStatus dataChunkStatus : dataChunkStatuses) { + String logFileName = String.format(logFilePattern, dataChunkStatus.getDataChunkId()); + Path dataChunkLogFile = tempDir.resolve(logFileName); + assertTrue(Files.exists(dataChunkLogFile), "Data chunk summary log file should exist"); + + // String logContent = Files.readString(dataChunkLogFile); + String logContent = new String(Files.readAllBytes(dataChunkLogFile), StandardCharsets.UTF_8); + DataLoaderObjectMapper objectMapper = new DataLoaderObjectMapper(); + List logEntries = + objectMapper.readValue(logContent, new TypeReference>() {}); + + assertEquals(1, logEntries.size()); + assertDataChunkStatusEquals(dataChunkStatus, logEntries.get(0)); + } + } + + private void assertDataChunkStatusEquals( + ImportDataChunkStatus expected, ImportDataChunkStatus actual) { + assertEquals(expected.getDataChunkId(), actual.getDataChunkId()); + assertEquals(expected.getStartTime(), actual.getStartTime()); + assertEquals(expected.getEndTime(), actual.getEndTime()); + assertEquals(expected.getTotalRecords(), actual.getTotalRecords()); + assertEquals(expected.getSuccessCount(), actual.getSuccessCount()); + assertEquals(expected.getFailureCount(), actual.getFailureCount()); + assertEquals(expected.getBatchCount(), actual.getBatchCount()); + assertEquals( + expected.getTotalDurationInMilliSeconds(), actual.getTotalDurationInMilliSeconds()); + } +} diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java new file mode 100644 index 0000000000..5182b97cb8 --- /dev/null +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java @@ -0,0 +1,67 @@ +package com.scalar.db.dataloader.core.dataimport.log.writer; + +import com.scalar.db.dataloader.core.dataimport.log.ImportLoggerConfig; +import com.scalar.db.dataloader.core.dataimport.log.LogStorageLocation; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import software.amazon.awssdk.services.s3.S3AsyncClient; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Paths; + +class DefaultLogWriterFactoryTest { + + String filePath = Paths.get("").toAbsolutePath() + "/sample.log"; + DefaultLogWriterFactory defaultLogWriterFactory; + + @AfterEach + void removeFileIfCreated() { + File file = new File(filePath); + if (file.exists()) { + file.deleteOnExit(); + } + } + + @Test + void createLogWriter_withValidLocalLogFilePath_shouldReturnLocalFileLogWriterObject() + throws IOException { + defaultLogWriterFactory = + new DefaultLogWriterFactory( + LogWriterFactoryConfig.builder() + .logStorageLocation(LogStorageLocation.LOCAL_FILE_STORAGE) + .build(), + ImportLoggerConfig.builder() + .prettyPrint(false) + .logSuccessRecords(false) + .logRawSourceRecords(false) + .logDirectoryPath("path") + .build()); + LogWriter logWriter = defaultLogWriterFactory.createLogWriter(filePath); + Assertions.assertEquals(LocalFileLogWriter.class, logWriter.getClass()); + logWriter.close(); + } + + @Test + void createLogWriter_withValidFilePath_shouldReturnLogWriterObject() throws IOException { + defaultLogWriterFactory = + new DefaultLogWriterFactory( + LogWriterFactoryConfig.builder() + .logStorageLocation(LogStorageLocation.AWS_S3) + .bucketName("bucket") + .objectKey("ObjectKay") + .s3AsyncClient(Mockito.mock(S3AsyncClient.class)) + .build(), + ImportLoggerConfig.builder() + .prettyPrint(false) + .logSuccessRecords(false) + .logRawSourceRecords(false) + .logDirectoryPath("path") + .build()); + LogWriter logWriter = defaultLogWriterFactory.createLogWriter(filePath); + Assertions.assertEquals(AwsS3LogWriter.class, logWriter.getClass()); + logWriter.close(); + } +} From 89b9f058fccb145350bfe6fd9ee91a99a513fe62 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Fri, 11 Apr 2025 09:27:47 +0530 Subject: [PATCH 02/30] Spotless applied again --- .../dataimport/log/AbstractImportLogger.java | 3 +-- .../log/SingleFileImportLogger.java | 3 +-- .../log/SplitByDataChunkImportLogger.java | 5 ++--- .../dataimport/log/writer/AwsS3LogWriter.java | 3 +-- .../log/writer/DefaultLogWriterFactory.java | 3 +-- .../log/writer/LocalFileLogWriter.java | 1 - .../core/dataimport/log/writer/LogWriter.java | 1 - .../log/SingleFileImportLoggerTest.java | 21 +++++++++---------- .../log/SplitByDataChunkImportLoggerTest.java | 15 +++++++------ .../writer/DefaultLogWriterFactoryTest.java | 7 +++---- 10 files changed, 26 insertions(+), 36 deletions(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java index eac36c802c..11a7493ca9 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java @@ -12,12 +12,11 @@ import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchStatus; -import lombok.RequiredArgsConstructor; - import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import lombok.RequiredArgsConstructor; @RequiredArgsConstructor public abstract class AbstractImportLogger implements ImportEventListener { diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java index e851631468..fc70770761 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java @@ -8,11 +8,10 @@ import com.scalar.db.dataloader.core.dataimport.task.result.ImportTargetResultStatus; import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; +import java.io.IOException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.IOException; - public class SingleFileImportLogger extends AbstractImportLogger { protected static final String SUMMARY_LOG_FILE_NAME = "summary.log"; diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java index f12e6e5f73..bec306ef9b 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java @@ -9,12 +9,11 @@ import com.scalar.db.dataloader.core.dataimport.task.result.ImportTargetResultStatus; import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import java.io.IOException; import java.util.HashMap; import java.util.Map; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class SplitByDataChunkImportLogger extends AbstractImportLogger { diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java index 857fc47a69..c11fab0b23 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java @@ -1,11 +1,10 @@ package com.scalar.db.dataloader.core.dataimport.log.writer; import com.fasterxml.jackson.databind.JsonNode; +import java.io.IOException; import lombok.AllArgsConstructor; import software.amazon.awssdk.services.s3.S3AsyncClient; -import java.io.IOException; - @AllArgsConstructor public class AwsS3LogWriter implements LogWriter { diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java index 6940f8d16a..27c1eb6c5f 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java @@ -1,9 +1,8 @@ package com.scalar.db.dataloader.core.dataimport.log.writer; import com.scalar.db.dataloader.core.dataimport.log.ImportLoggerConfig; -import lombok.AllArgsConstructor; - import java.io.IOException; +import lombok.AllArgsConstructor; /** A factory class to create log writers. */ @AllArgsConstructor diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java index eb152b6e1d..b29395e8ec 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java @@ -4,7 +4,6 @@ import com.fasterxml.jackson.databind.JsonNode; import com.scalar.db.dataloader.core.DataLoaderObjectMapper; import com.scalar.db.dataloader.core.dataimport.log.ImportLoggerConfig; - import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java index cfd713acc3..f10917901f 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java @@ -1,7 +1,6 @@ package com.scalar.db.dataloader.core.dataimport.log.writer; import com.fasterxml.jackson.databind.JsonNode; - import java.io.IOException; public interface LogWriter extends AutoCloseable { diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java index cd80cf61d1..98e58109e7 100644 --- a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java @@ -1,5 +1,9 @@ package com.scalar.db.dataloader.core.dataimport.log; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + import com.fasterxml.jackson.core.type.TypeReference; import com.scalar.db.dataloader.core.DataLoaderObjectMapper; import com.scalar.db.dataloader.core.dataimport.datachunk.ImportDataChunkStatus; @@ -8,13 +12,6 @@ import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactoryConfig; import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.io.TempDir; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; @@ -25,10 +22,12 @@ import java.util.List; import java.util.stream.Collectors; import java.util.stream.Stream; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; class SingleFileImportLoggerTest { diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java index e1f397caf5..800ae4e97a 100644 --- a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java @@ -1,5 +1,9 @@ package com.scalar.db.dataloader.core.dataimport.log; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + import com.fasterxml.jackson.core.type.TypeReference; import com.scalar.db.dataloader.core.DataLoaderObjectMapper; import com.scalar.db.dataloader.core.dataimport.datachunk.ImportDataChunkStatus; @@ -8,10 +12,6 @@ import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactoryConfig; import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.io.TempDir; - import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; @@ -22,10 +22,9 @@ import java.util.List; import java.util.stream.Collectors; import java.util.stream.IntStream; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; class SplitByDataChunkImportLoggerTest { diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java index 5182b97cb8..e9102bca61 100644 --- a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java @@ -2,16 +2,15 @@ import com.scalar.db.dataloader.core.dataimport.log.ImportLoggerConfig; import com.scalar.db.dataloader.core.dataimport.log.LogStorageLocation; +import java.io.File; +import java.io.IOException; +import java.nio.file.Paths; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.mockito.Mockito; import software.amazon.awssdk.services.s3.S3AsyncClient; -import java.io.File; -import java.io.IOException; -import java.nio.file.Paths; - class DefaultLogWriterFactoryTest { String filePath = Paths.get("").toAbsolutePath() + "/sample.log"; From 49c83b69ed58b70003098bbf149cea3af1accba7 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Fri, 11 Apr 2025 11:08:01 +0530 Subject: [PATCH 03/30] Removed unused code --- .../core/dataimport/log/AbstractImportLogger.java | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java index 11a7493ca9..5faf8419dd 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java @@ -27,14 +27,6 @@ public abstract class AbstractImportLogger implements ImportEventListener { protected final LogWriterFactory logWriterFactory; protected final List listeners = new ArrayList<>(); - public void addListener(ImportEventListener listener) { - listeners.add(listener); - } - - public void removeListener(ImportEventListener listener) { - listeners.remove(listener); - } - @Override public void onDataChunkStarted(ImportDataChunkStatus importDataChunkStatus) { // Currently we are not logging the start of a data chunk @@ -57,11 +49,6 @@ public void onTransactionBatchCompleted(ImportTransactionBatchResult batchResult notifyTransactionBatchCompleted(batchResult); } - @Override - public void onTaskComplete(ImportTaskResult taskResult) { - // TODO: we can remove this event if it's current not being used in the import Manager as well - } - protected abstract void logTransactionBatch(ImportTransactionBatchResult batchResult); protected boolean shouldSkipLoggingSuccess(ImportTransactionBatchResult batchResult) { From c5c9c0aad87ad67543ab999dfd4d5540f621e8e6 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Tue, 15 Apr 2025 11:45:17 +0530 Subject: [PATCH 04/30] Removed unused classes and references --- data-loader/build.gradle | 1 - .../dataimport/log/LogStorageLocation.java | 7 ----- .../dataimport/log/writer/AwsS3LogWriter.java | 29 ------------------- .../log/writer/DefaultLogWriterFactory.java | 16 ++-------- .../log/writer/LogWriterFactoryConfig.java | 15 ---------- .../log/SingleFileImportLoggerTest.java | 7 +---- .../log/SplitByDataChunkImportLoggerTest.java | 7 +---- .../writer/DefaultLogWriterFactoryTest.java | 27 ----------------- 8 files changed, 4 insertions(+), 105 deletions(-) delete mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/LogStorageLocation.java delete mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java delete mode 100644 data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactoryConfig.java diff --git a/data-loader/build.gradle b/data-loader/build.gradle index 5e9c2a4ba0..87a057933b 100644 --- a/data-loader/build.gradle +++ b/data-loader/build.gradle @@ -17,7 +17,6 @@ subprojects { implementation("org.apache.commons:commons-lang3:${commonsLangVersion}") implementation("commons-io:commons-io:${commonsIoVersion}") implementation("org.slf4j:slf4j-simple:${slf4jVersion}") - implementation("software.amazon.awssdk:s3:${awssdkVersion}") // Mockito testImplementation "org.mockito:mockito-core:${mockitoVersion}" diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/LogStorageLocation.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/LogStorageLocation.java deleted file mode 100644 index 396cb3d8e4..0000000000 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/LogStorageLocation.java +++ /dev/null @@ -1,7 +0,0 @@ -package com.scalar.db.dataloader.core.dataimport.log; - -/** The location where the logs are stored. */ -public enum LogStorageLocation { - LOCAL_FILE_STORAGE, - AWS_S3 -} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java deleted file mode 100644 index c11fab0b23..0000000000 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/AwsS3LogWriter.java +++ /dev/null @@ -1,29 +0,0 @@ -package com.scalar.db.dataloader.core.dataimport.log.writer; - -import com.fasterxml.jackson.databind.JsonNode; -import java.io.IOException; -import lombok.AllArgsConstructor; -import software.amazon.awssdk.services.s3.S3AsyncClient; - -@AllArgsConstructor -public class AwsS3LogWriter implements LogWriter { - - private final S3AsyncClient s3AsyncClient; - private final String bucketName; - private final String objectKey; - - @Override - public void write(JsonNode sourceRecord) throws IOException { - // Implementation to write content to cloud storage - } - - @Override - public void flush() throws IOException { - // Implementation to flush content to cloud storage - } - - @Override - public void close() throws IOException { - // Implementation to close the cloud storage connection - } -} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java index 27c1eb6c5f..5b2dc77497 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java @@ -8,28 +8,16 @@ @AllArgsConstructor public class DefaultLogWriterFactory implements LogWriterFactory { - private final LogWriterFactoryConfig config; private final ImportLoggerConfig importLoggerConfig; /** - * Creates a log writer based on the configuration. + * Creates a log writer object * * @param logFilePath the path of the log file * @return the log writer */ @Override public LogWriter createLogWriter(String logFilePath) throws IOException { - LogWriter logWriter = null; - switch (config.getLogStorageLocation()) { - case LOCAL_FILE_STORAGE: - logWriter = new LocalFileLogWriter(logFilePath, importLoggerConfig); - break; - case AWS_S3: - logWriter = - new AwsS3LogWriter( - config.getS3AsyncClient(), config.getBucketName(), config.getObjectKey()); - break; - } - return logWriter; + return new LocalFileLogWriter(logFilePath, importLoggerConfig); } } diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactoryConfig.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactoryConfig.java deleted file mode 100644 index 901d0aae6f..0000000000 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactoryConfig.java +++ /dev/null @@ -1,15 +0,0 @@ -package com.scalar.db.dataloader.core.dataimport.log.writer; - -import com.scalar.db.dataloader.core.dataimport.log.LogStorageLocation; -import lombok.Builder; -import lombok.Value; -import software.amazon.awssdk.services.s3.S3AsyncClient; - -@Builder -@Value -public class LogWriterFactoryConfig { - LogStorageLocation logStorageLocation; - S3AsyncClient s3AsyncClient; - String bucketName; - String objectKey; -} diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java index 98e58109e7..e2d17dfa4f 100644 --- a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java @@ -9,7 +9,6 @@ import com.scalar.db.dataloader.core.dataimport.datachunk.ImportDataChunkStatus; import com.scalar.db.dataloader.core.dataimport.log.writer.DefaultLogWriterFactory; import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactory; -import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactoryConfig; import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; import java.io.IOException; @@ -40,10 +39,6 @@ class SingleFileImportLoggerTest { @BeforeEach void setUp() { - LogWriterFactoryConfig logWriterFactoryConfig = - LogWriterFactoryConfig.builder() - .logStorageLocation(LogStorageLocation.LOCAL_FILE_STORAGE) - .build(); ImportLoggerConfig importLoggerConfig = ImportLoggerConfig.builder() .prettyPrint(false) @@ -51,7 +46,7 @@ void setUp() { .logRawSourceRecords(false) .logDirectoryPath("path") .build(); - logWriterFactory = new DefaultLogWriterFactory(logWriterFactoryConfig, importLoggerConfig); + logWriterFactory = new DefaultLogWriterFactory(importLoggerConfig); } @AfterEach diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java index 800ae4e97a..04d9906641 100644 --- a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java @@ -9,7 +9,6 @@ import com.scalar.db.dataloader.core.dataimport.datachunk.ImportDataChunkStatus; import com.scalar.db.dataloader.core.dataimport.log.writer.DefaultLogWriterFactory; import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactory; -import com.scalar.db.dataloader.core.dataimport.log.writer.LogWriterFactoryConfig; import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; import java.io.IOException; @@ -36,10 +35,6 @@ class SplitByDataChunkImportLoggerTest { @BeforeEach void setUp() { - LogWriterFactoryConfig logWriterFactoryConfig = - LogWriterFactoryConfig.builder() - .logStorageLocation(LogStorageLocation.LOCAL_FILE_STORAGE) - .build(); ImportLoggerConfig importLoggerConfig = ImportLoggerConfig.builder() .prettyPrint(false) @@ -47,7 +42,7 @@ void setUp() { .logRawSourceRecords(false) .logDirectoryPath("path") .build(); - logWriterFactory = new DefaultLogWriterFactory(logWriterFactoryConfig, importLoggerConfig); + logWriterFactory = new DefaultLogWriterFactory(importLoggerConfig); } @Test diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java index e9102bca61..3b99510a7e 100644 --- a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java @@ -1,15 +1,12 @@ package com.scalar.db.dataloader.core.dataimport.log.writer; import com.scalar.db.dataloader.core.dataimport.log.ImportLoggerConfig; -import com.scalar.db.dataloader.core.dataimport.log.LogStorageLocation; import java.io.File; import java.io.IOException; import java.nio.file.Paths; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; -import org.mockito.Mockito; -import software.amazon.awssdk.services.s3.S3AsyncClient; class DefaultLogWriterFactoryTest { @@ -29,9 +26,6 @@ void createLogWriter_withValidLocalLogFilePath_shouldReturnLocalFileLogWriterObj throws IOException { defaultLogWriterFactory = new DefaultLogWriterFactory( - LogWriterFactoryConfig.builder() - .logStorageLocation(LogStorageLocation.LOCAL_FILE_STORAGE) - .build(), ImportLoggerConfig.builder() .prettyPrint(false) .logSuccessRecords(false) @@ -42,25 +36,4 @@ void createLogWriter_withValidLocalLogFilePath_shouldReturnLocalFileLogWriterObj Assertions.assertEquals(LocalFileLogWriter.class, logWriter.getClass()); logWriter.close(); } - - @Test - void createLogWriter_withValidFilePath_shouldReturnLogWriterObject() throws IOException { - defaultLogWriterFactory = - new DefaultLogWriterFactory( - LogWriterFactoryConfig.builder() - .logStorageLocation(LogStorageLocation.AWS_S3) - .bucketName("bucket") - .objectKey("ObjectKay") - .s3AsyncClient(Mockito.mock(S3AsyncClient.class)) - .build(), - ImportLoggerConfig.builder() - .prettyPrint(false) - .logSuccessRecords(false) - .logRawSourceRecords(false) - .logDirectoryPath("path") - .build()); - LogWriter logWriter = defaultLogWriterFactory.createLogWriter(filePath); - Assertions.assertEquals(AwsS3LogWriter.class, logWriter.getClass()); - logWriter.close(); - } } From 3934c2a1a6a4ccdc3e6ffbb5e2ecb01bf859381f Mon Sep 17 00:00:00 2001 From: Peckstadt Yves Date: Wed, 16 Apr 2025 16:30:47 +0900 Subject: [PATCH 05/30] Improve Javadocs --- .../dataimport/log/AbstractImportLogger.java | 87 +++++++++++++++ .../dataimport/log/ImportLoggerConfig.java | 24 ++++ .../log/SingleFileImportLogger.java | 94 ++++++++++++++++ .../log/SplitByDataChunkImportLogger.java | 105 ++++++++++++++++++ .../log/writer/DefaultLogWriterFactory.java | 16 ++- .../log/writer/LocalFileLogWriter.java | 30 ++++- .../dataimport/log/writer/LogFileType.java | 19 +++- .../core/dataimport/log/writer/LogWriter.java | 23 ++++ .../log/writer/LogWriterFactory.java | 12 ++ 9 files changed, 402 insertions(+), 8 deletions(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java index 5faf8419dd..4a6121a97b 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java @@ -18,26 +18,56 @@ import java.util.List; import lombok.RequiredArgsConstructor; +/** + * An abstract base class for logging import events during data loading operations. This class + * implements the {@link ImportEventListener} interface and provides common functionality for + * logging transaction batch results and managing event listeners. Concrete implementations should + * define how to log transaction batches and handle errors. + */ @RequiredArgsConstructor public abstract class AbstractImportLogger implements ImportEventListener { + /** Object mapper used for JSON serialization/deserialization. */ protected static final DataLoaderObjectMapper OBJECT_MAPPER = new DataLoaderObjectMapper(); + /** Configuration for the import logger. */ protected final ImportLoggerConfig config; + + /** Factory for creating log writers. */ protected final LogWriterFactory logWriterFactory; + + /** List of event listeners to be notified of import events. */ protected final List listeners = new ArrayList<>(); + /** + * Called when a data chunk import is started. Currently, this implementation does not log the + * start of a data chunk. + * + * @param importDataChunkStatus the status of the data chunk being imported + */ @Override public void onDataChunkStarted(ImportDataChunkStatus importDataChunkStatus) { // Currently we are not logging the start of a data chunk } + /** + * Called when a transaction batch is started. Currently, this implementation does not log the + * start of a transaction batch, but it notifies all registered listeners. + * + * @param batchStatus the status of the transaction batch being started + */ @Override public void onTransactionBatchStarted(ImportTransactionBatchStatus batchStatus) { // Currently we are not logging the start of a transaction batch notifyTransactionBatchStarted(batchStatus); } + /** + * Called when a transaction batch is completed. This method logs the transaction batch result if + * it should be logged based on the configuration, and notifies all registered listeners. + * + * @param batchResult the result of the completed transaction batch + */ @Override public void onTransactionBatchCompleted(ImportTransactionBatchResult batchResult) { // skip logging success records if the configuration is set to skip @@ -49,12 +79,32 @@ public void onTransactionBatchCompleted(ImportTransactionBatchResult batchResult notifyTransactionBatchCompleted(batchResult); } + /** + * Logs a transaction batch result. This method should be implemented by concrete subclasses to + * define how to log transaction batch results. + * + * @param batchResult the transaction batch result to log + */ protected abstract void logTransactionBatch(ImportTransactionBatchResult batchResult); + /** + * Determines whether logging of a successful transaction batch should be skipped. Logging is + * skipped if the batch was successful and the configuration specifies not to log success records. + * + * @param batchResult the transaction batch result to check + * @return true if logging should be skipped, false otherwise + */ protected boolean shouldSkipLoggingSuccess(ImportTransactionBatchResult batchResult) { return batchResult.isSuccess() && !config.isLogSuccessRecords(); } + /** + * Creates a filtered JSON representation of a transaction batch result. This method filters out + * raw record data if the configuration specifies not to log raw source records. + * + * @param batchResult the transaction batch result to convert to JSON + * @return a JsonNode representing the filtered transaction batch result + */ protected JsonNode createFilteredTransactionBatchLogJsonNode( ImportTransactionBatchResult batchResult) { @@ -105,6 +155,12 @@ protected JsonNode createFilteredTransactionBatchLogJsonNode( return OBJECT_MAPPER.valueToTree(modifiedBatchResult); } + /** + * Safely closes a log writer. If an IOException occurs during closing, it logs the error using + * the {@link #logError} method. + * + * @param logWriter the log writer to close, may be null + */ protected void closeLogWriter(LogWriter logWriter) { if (logWriter != null) { try { @@ -115,24 +171,55 @@ protected void closeLogWriter(LogWriter logWriter) { } } + /** + * Logs an error message and exception. This method should be implemented by concrete subclasses + * to define how to log errors. + * + * @param errorMessage the error message to log + * @param e the exception that caused the error + */ protected abstract void logError(String errorMessage, Exception e); + /** + * Creates a log writer for the specified log file path. + * + * @param logFilePath the path to the log file + * @return a new log writer + * @throws IOException if an I/O error occurs while creating the log writer + */ protected LogWriter createLogWriter(String logFilePath) throws IOException { return logWriterFactory.createLogWriter(logFilePath); } + /** + * Notifies all registered listeners that a transaction batch has started. + * + * @param status the status of the transaction batch that has started + */ private void notifyTransactionBatchStarted(ImportTransactionBatchStatus status) { for (ImportEventListener listener : listeners) { listener.onTransactionBatchStarted(status); } } + /** + * Notifies all registered listeners that a transaction batch has completed. + * + * @param batchResult the result of the completed transaction batch + */ private void notifyTransactionBatchCompleted(ImportTransactionBatchResult batchResult) { for (ImportEventListener listener : listeners) { listener.onTransactionBatchCompleted(batchResult); } } + /** + * Updates the status of target results for an aborted transaction batch. For each target with a + * status of SAVED, changes the status to ABORTED and adds an error message. + * + * @param targetResults the list of target results to update + * @return the updated list of target results + */ private List updateTargetStatusForAbortedTransactionBatch( List targetResults) { for (int i = 0; i < targetResults.size(); i++) { diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java index fc0039bf90..9e0033fb26 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java @@ -3,11 +3,35 @@ import lombok.Builder; import lombok.Value; +/** + * Configuration class for import loggers. This class uses Lombok's {@code @Value} annotation to + * create an immutable class and {@code @Builder} annotation to provide a builder pattern for + * creating instances. + */ @Value @Builder public class ImportLoggerConfig { + /** + * The directory path where log files will be stored. This path should end with a directory + * separator (e.g., "/"). + */ String logDirectoryPath; + + /** + * Whether to log records that were successfully imported. If true, successful import operations + * will be logged to success log files. + */ boolean logSuccessRecords; + + /** + * Whether to log raw source records that failed to be imported. If true, failed import operations + * will be logged to failure log files. + */ boolean logRawSourceRecords; + + /** + * Whether to format the logs with pretty printing. If true, the JSON logs will be formatted with + * indentation for better readability. + */ boolean prettyPrint; } diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java index fc70770761..f5a0a133e9 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java @@ -12,6 +12,20 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +/** + * An implementation of {@link AbstractImportLogger} that uses a single file for each log type. + * Unlike {@link SplitByDataChunkImportLogger}, this logger creates only three log files: one for + * successful operations, one for failed operations, and one for summary information, regardless of + * the number of data chunks processed. + * + *

The log files are named as follows: + * + *

    + *
  • success.json - Records of successful import operations + *
  • failure.json - Records of failed import operations + *
  • summary.log - Summary information for all data chunks + *
+ */ public class SingleFileImportLogger extends AbstractImportLogger { protected static final String SUMMARY_LOG_FILE_NAME = "summary.log"; @@ -22,6 +36,15 @@ public class SingleFileImportLogger extends AbstractImportLogger { private LogWriter successLogWriter; private LogWriter failureLogWriter; + /** + * Creates a new instance of SingleFileImportLogger. Initializes the success and failure log + * writers immediately. The summary log writer is created on demand when the first data chunk is + * completed. + * + * @param config the configuration for the logger + * @param logWriterFactory the factory to create log writers + * @throws IOException if an I/O error occurs while creating the log writers + */ public SingleFileImportLogger(ImportLoggerConfig config, LogWriterFactory logWriterFactory) throws IOException { super(config, logWriterFactory); @@ -29,6 +52,12 @@ public SingleFileImportLogger(ImportLoggerConfig config, LogWriterFactory logWri failureLogWriter = createLogWriter(config.getLogDirectoryPath() + FAILURE_LOG_FILE_NAME); } + /** + * Called when an import task is completed. Writes the task result details to the appropriate log + * files based on the configuration. + * + * @param taskResult the result of the completed import task + */ @Override public void onTaskComplete(ImportTaskResult taskResult) { if (!config.isLogSuccessRecords() && !config.isLogRawSourceRecords()) return; @@ -39,9 +68,21 @@ public void onTaskComplete(ImportTaskResult taskResult) { } } + /** + * Called to add or update the status of a data chunk. This implementation does nothing as the + * status is only logged when the data chunk is completed. + * + * @param status the status of the data chunk + */ @Override public void addOrUpdateDataChunkStatus(ImportDataChunkStatus status) {} + /** + * Called when a data chunk is completed. Logs the summary of the data chunk to the summary log + * file. + * + * @param dataChunkStatus the status of the completed data chunk + */ @Override public void onDataChunkCompleted(ImportDataChunkStatus dataChunkStatus) { try { @@ -51,11 +92,17 @@ public void onDataChunkCompleted(ImportDataChunkStatus dataChunkStatus) { } } + /** Called when all data chunks are completed. Closes all log writers. */ @Override public void onAllDataChunksCompleted() { closeAllLogWriters(); } + /** + * Logs a transaction batch result to the appropriate log file based on its success status. + * + * @param batchResult the transaction batch result to log + */ @Override protected void logTransactionBatch(ImportTransactionBatchResult batchResult) { try { @@ -67,11 +114,24 @@ protected void logTransactionBatch(ImportTransactionBatchResult batchResult) { } } + /** + * Logs an error message with an exception to the logger. + * + * @param errorMessage the error message to log + * @param exception the exception associated with the error + */ @Override protected void logError(String errorMessage, Exception exception) { LOGGER.error(errorMessage, exception); } + /** + * Logs the summary of a data chunk to the summary log file. Creates the summary log writer if it + * doesn't exist yet. + * + * @param dataChunkStatus the status of the data chunk to log + * @throws IOException if an I/O error occurs while writing to the log + */ private void logDataChunkSummary(ImportDataChunkStatus dataChunkStatus) throws IOException { if (summaryLogWriter == null) { summaryLogWriter = createLogWriter(config.getLogDirectoryPath() + SUMMARY_LOG_FILE_NAME); @@ -79,12 +139,27 @@ private void logDataChunkSummary(ImportDataChunkStatus dataChunkStatus) throws I writeImportDataChunkSummary(dataChunkStatus, summaryLogWriter); } + /** + * Writes the summary of a data chunk to the specified log writer. + * + * @param dataChunkStatus the status of the data chunk to log + * @param logWriter the log writer to write to + * @throws IOException if an I/O error occurs while writing to the log + */ private void writeImportDataChunkSummary( ImportDataChunkStatus dataChunkStatus, LogWriter logWriter) throws IOException { JsonNode jsonNode = OBJECT_MAPPER.valueToTree(dataChunkStatus); writeToLogWriter(logWriter, jsonNode); } + /** + * Gets the appropriate log writer for a transaction batch based on its success status. If the log + * writer doesn't exist yet, it will be created. + * + * @param batchResult the transaction batch result + * @return the log writer for the batch + * @throws IOException if an I/O error occurs while creating a new log writer + */ private LogWriter getLogWriterForTransactionBatch(ImportTransactionBatchResult batchResult) throws IOException { String logFileName = batchResult.isSuccess() ? SUCCESS_LOG_FILE_NAME : FAILURE_LOG_FILE_NAME; @@ -100,6 +175,14 @@ private LogWriter getLogWriterForTransactionBatch(ImportTransactionBatchResult b return logWriter; } + /** + * Writes the details of an import task result to the appropriate log files. Successful targets + * are written to success logs and failed targets to failure logs. The method is synchronized on + * the respective log writers to ensure thread safety. + * + * @param importTaskResult the result of the import task to log + * @throws IOException if an I/O error occurs while writing to the logs + */ private void writeImportTaskResultDetailToLogs(ImportTaskResult importTaskResult) throws IOException { JsonNode jsonNode; @@ -123,11 +206,22 @@ private void writeImportTaskResultDetailToLogs(ImportTaskResult importTaskResult } } + /** + * Writes a JSON node to a log writer and flushes the writer. + * + * @param logWriter the log writer to write to + * @param jsonNode the JSON node to write + * @throws IOException if an I/O error occurs while writing + */ private void writeToLogWriter(LogWriter logWriter, JsonNode jsonNode) throws IOException { logWriter.write(jsonNode); logWriter.flush(); } + /** + * Closes all log writers and sets them to null. This method is called when all data chunks have + * been completed. + */ private void closeAllLogWriters() { closeLogWriter(summaryLogWriter); closeLogWriter(successLogWriter); diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java index bec306ef9b..69deaf9445 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java @@ -15,6 +15,22 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +/** + * An implementation of {@link AbstractImportLogger} that creates separate log files for each data + * chunk. This logger maintains separate log writers for success, failure, and summary logs for each + * data chunk, allowing for better organization and easier tracking of import operations by data + * chunk. + * + *

The log files are named using the following formats: + * + *

    + *
  • Success logs: data_chunk_[id]_success.json + *
  • Failure logs: data_chunk_[id]_failure.json + *
  • Summary logs: data_chunk_[id]_summary.json + *
+ * + *

Log writers are created on demand and closed when their corresponding data chunk is completed. + */ public class SplitByDataChunkImportLogger extends AbstractImportLogger { protected static final String SUMMARY_LOG_FILE_NAME_FORMAT = "data_chunk_%s_summary.json"; @@ -26,11 +42,23 @@ public class SplitByDataChunkImportLogger extends AbstractImportLogger { private final Map successLogWriters = new HashMap<>(); private final Map failureLogWriters = new HashMap<>(); + /** + * Creates a new instance of SplitByDataChunkImportLogger. + * + * @param config the configuration for the logger + * @param logWriterFactory the factory to create log writers + */ public SplitByDataChunkImportLogger( ImportLoggerConfig config, LogWriterFactory logWriterFactory) { super(config, logWriterFactory); } + /** + * Called when an import task is completed. Writes the task result details to the appropriate log + * files based on the configuration. + * + * @param taskResult the result of the completed import task + */ @Override public void onTaskComplete(ImportTaskResult taskResult) { if (!config.isLogSuccessRecords() && !config.isLogRawSourceRecords()) return; @@ -41,6 +69,13 @@ public void onTaskComplete(ImportTaskResult taskResult) { } } + /** + * Writes the details of an import task result to the appropriate log files. Successful targets + * are written to success logs and failed targets to failure logs. + * + * @param importTaskResult the result of the import task to log + * @throws IOException if an I/O error occurs while writing to the logs + */ private void writeImportTaskResultDetailToLogs(ImportTaskResult importTaskResult) throws IOException { JsonNode jsonNode; @@ -68,9 +103,21 @@ private void writeImportTaskResultDetailToLogs(ImportTaskResult importTaskResult } } + /** + * Called to add or update the status of a data chunk. This implementation does nothing as the + * status is only logged when the data chunk is completed. + * + * @param status the status of the data chunk + */ @Override public void addOrUpdateDataChunkStatus(ImportDataChunkStatus status) {} + /** + * Called when a data chunk is completed. Logs the summary of the data chunk and closes the log + * writers for that data chunk. + * + * @param dataChunkStatus the status of the completed data chunk + */ @Override public void onDataChunkCompleted(ImportDataChunkStatus dataChunkStatus) { try { @@ -82,11 +129,18 @@ public void onDataChunkCompleted(ImportDataChunkStatus dataChunkStatus) { } } + /** Called when all data chunks are completed. Closes all remaining log writers. */ @Override public void onAllDataChunksCompleted() { closeAllDataChunkLogWriters(); } + /** + * Logs a transaction batch result to the appropriate log file based on its success status. The + * log file is determined by the data chunk ID and whether the batch was successful. + * + * @param batchResult the transaction batch result to log + */ @Override protected void logTransactionBatch(ImportTransactionBatchResult batchResult) { LogFileType logFileType = batchResult.isSuccess() ? LogFileType.SUCCESS : LogFileType.FAILURE; @@ -102,11 +156,23 @@ protected void logTransactionBatch(ImportTransactionBatchResult batchResult) { } } + /** + * Logs an error message with an exception to the logger. + * + * @param errorMessage the error message to log + * @param exception the exception associated with the error + */ @Override protected void logError(String errorMessage, Exception exception) { LOGGER.error(errorMessage, exception); } + /** + * Logs the summary of a data chunk to a summary log file. + * + * @param dataChunkStatus the status of the data chunk to log + * @throws IOException if an I/O error occurs while writing to the log + */ private void logDataChunkSummary(ImportDataChunkStatus dataChunkStatus) throws IOException { try (LogWriter logWriter = initializeLogWriterIfNeeded(LogFileType.SUMMARY, dataChunkStatus.getDataChunkId())) { @@ -115,12 +181,21 @@ private void logDataChunkSummary(ImportDataChunkStatus dataChunkStatus) throws I } } + /** + * Closes and removes the log writers for a specific data chunk. + * + * @param dataChunkId the ID of the data chunk whose log writers should be closed + */ private void closeLogWritersForDataChunk(int dataChunkId) { closeLogWriter(successLogWriters.remove(dataChunkId)); closeLogWriter(failureLogWriters.remove(dataChunkId)); closeLogWriter(summaryLogWriters.remove(dataChunkId)); } + /** + * Closes all log writers for all data chunks and clears the writer maps. This method is called + * when all data chunks have been completed. + */ private void closeAllDataChunkLogWriters() { summaryLogWriters.values().forEach(this::closeLogWriter); successLogWriters.values().forEach(this::closeLogWriter); @@ -130,6 +205,13 @@ private void closeAllDataChunkLogWriters() { failureLogWriters.clear(); } + /** + * Constructs the log file path based on the batch ID and log file type. + * + * @param batchId the ID of the batch (data chunk) + * @param logFileType the type of log file (SUCCESS, FAILURE, or SUMMARY) + * @return the full path to the log file + */ private String getLogFilePath(long batchId, LogFileType logFileType) { String logfilePath; switch (logFileType) { @@ -151,6 +233,15 @@ private String getLogFilePath(long batchId, LogFileType logFileType) { return logfilePath; } + /** + * Gets or creates a log writer for the specified log file type and data chunk ID. If a log writer + * for the specified type and data chunk doesn't exist, it will be created. + * + * @param logFileType the type of log file + * @param dataChunkId the ID of the data chunk + * @return the log writer for the specified type and data chunk + * @throws IOException if an I/O error occurs while creating a new log writer + */ private LogWriter initializeLogWriterIfNeeded(LogFileType logFileType, int dataChunkId) throws IOException { Map logWriters = getLogWriters(logFileType); @@ -161,11 +252,25 @@ private LogWriter initializeLogWriterIfNeeded(LogFileType logFileType, int dataC return logWriters.get(dataChunkId); } + /** + * Creates a new log writer for the specified log file type and data chunk ID. + * + * @param logFileType the type of log file + * @param dataChunkId the ID of the data chunk + * @return a new log writer + * @throws IOException if an I/O error occurs while creating the log writer + */ private LogWriter createLogWriter(LogFileType logFileType, int dataChunkId) throws IOException { String logFilePath = getLogFilePath(dataChunkId, logFileType); return createLogWriter(logFilePath); } + /** + * Gets the appropriate map of log writers for the specified log file type. + * + * @param logFileType the type of log file + * @return the map of log writers for the specified type + */ private Map getLogWriters(LogFileType logFileType) { Map logWriterMap = null; switch (logFileType) { diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java index 5b2dc77497..b85ee8a33b 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactory.java @@ -4,17 +4,25 @@ import java.io.IOException; import lombok.AllArgsConstructor; -/** A factory class to create log writers. */ +/** + * The default implementation of {@link LogWriterFactory} that creates {@link LocalFileLogWriter} + * instances. This factory uses the provided {@link ImportLoggerConfig} to configure the log writers + * it creates. It's annotated with Lombok's {@code @AllArgsConstructor} to automatically generate a + * constructor that initializes the configuration field. + */ @AllArgsConstructor public class DefaultLogWriterFactory implements LogWriterFactory { private final ImportLoggerConfig importLoggerConfig; /** - * Creates a log writer object + * Creates a {@link LocalFileLogWriter} for the specified log file path. The created log writer + * will be configured using the {@link ImportLoggerConfig} that was provided to this factory + * during construction. * - * @param logFilePath the path of the log file - * @return the log writer + * @param logFilePath the path where the log file will be created or appended to + * @return a new {@link LogWriter} instance that writes to the specified file + * @throws IOException if an I/O error occurs while creating the log writer */ @Override public LogWriter createLogWriter(String logFilePath) throws IOException { diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java index b29395e8ec..323c02661b 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java @@ -10,15 +10,21 @@ import java.nio.file.Paths; import java.nio.file.StandardOpenOption; +/** + * An implementation of {@link LogWriter} that writes log entries to a local file. This class writes + * JSON records to a file as a JSON array, with each record being an element in the array. It + * handles file creation, appending, and proper JSON formatting. + */ public class LocalFileLogWriter implements LogWriter { private final JsonGenerator logWriter; private final DataLoaderObjectMapper objectMapper; /** - * Creates an instance of LocalFileLogWriter with the specified file path and log file type. + * Creates an instance of LocalFileLogWriter with the specified file path and configuration. * - * @param filePath the file path - * @throws IOException if an I/O error occurs + * @param filePath the path where the log file will be created or appended to + * @param importLoggerConfig the configuration for the logger, including formatting options + * @throws IOException if an I/O error occurs while creating or opening the file */ public LocalFileLogWriter(String filePath, ImportLoggerConfig importLoggerConfig) throws IOException { @@ -36,6 +42,13 @@ public LocalFileLogWriter(String filePath, ImportLoggerConfig importLoggerConfig this.logWriter.flush(); } + /** + * Writes a JSON record to the log file. If the source record is null, this method does nothing. + * The method is synchronized to ensure thread safety when writing to the file. + * + * @param sourceRecord the JSON record to write + * @throws IOException if an I/O error occurs while writing the record + */ @Override public void write(JsonNode sourceRecord) throws IOException { if (sourceRecord == null) { @@ -46,11 +59,22 @@ public void write(JsonNode sourceRecord) throws IOException { } } + /** + * Flushes any buffered data to the log file. + * + * @throws IOException if an I/O error occurs while flushing + */ @Override public void flush() throws IOException { logWriter.flush(); } + /** + * Closes the log writer, properly ending the JSON array and releasing resources. If the writer is + * already closed, this method does nothing. + * + * @throws IOException if an I/O error occurs while closing the writer + */ @Override public void close() throws IOException { if (logWriter.isClosed()) { diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogFileType.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogFileType.java index 5483aefc91..e56a949dad 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogFileType.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogFileType.java @@ -1,8 +1,25 @@ package com.scalar.db.dataloader.core.dataimport.log.writer; -/** The type of the log writer. */ +/** + * Represents the different types of log files used in the data import process. Each type serves a + * specific purpose in tracking the import operation's results. + */ public enum LogFileType { + /** + * Represents a log file that records successful import operations. These logs contain records + * that were successfully processed and imported. + */ SUCCESS, + + /** + * Represents a log file that records failed import operations. These logs contain records that + * failed to be processed or imported, along with information about the failure. + */ FAILURE, + + /** + * Represents a log file that provides a summary of the import operation. These logs contain + * aggregated statistics and overall results of the import process. + */ SUMMARY } diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java index f10917901f..32838f3215 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriter.java @@ -3,12 +3,35 @@ import com.fasterxml.jackson.databind.JsonNode; import java.io.IOException; +/** + * An interface for writing log entries to a destination. This interface extends {@link + * AutoCloseable} to ensure proper resource cleanup. Implementations of this interface handle the + * details of writing log entries to various destinations such as files, databases, or other storage + * systems. + */ public interface LogWriter extends AutoCloseable { + /** + * Writes a JSON record to the log. + * + * @param sourceRecord the JSON record to write + * @throws IOException if an I/O error occurs while writing the record + */ void write(JsonNode sourceRecord) throws IOException; + /** + * Flushes any buffered data to the underlying storage. + * + * @throws IOException if an I/O error occurs while flushing + */ void flush() throws IOException; + /** + * Closes this log writer and releases any system resources associated with it. This method should + * be called when the log writer is no longer needed to ensure proper cleanup of resources. + * + * @throws IOException if an I/O error occurs while closing the log writer + */ @Override void close() throws IOException; } diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactory.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactory.java index b3c4dfc080..3854d728c9 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactory.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LogWriterFactory.java @@ -2,7 +2,19 @@ import java.io.IOException; +/** + * A factory interface for creating {@link LogWriter} instances. This interface abstracts the + * creation of log writers, allowing different implementations to create different types of log + * writers based on the application's needs. + */ public interface LogWriterFactory { + /** + * Creates a new log writer for the specified log file path. + * + * @param logFilePath the path where the log file will be created or appended to + * @return a new {@link LogWriter} instance + * @throws IOException if an I/O error occurs while creating the log writer + */ LogWriter createLogWriter(String logFilePath) throws IOException; } From 9958f95ab6d49616ea897356d5c506ecfa53348d Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Mon, 21 Apr 2025 17:19:17 +0530 Subject: [PATCH 06/30] Changes --- .../log/SingleFileImportLogger.java | 51 +++++++----- .../log/SplitByDataChunkImportLogger.java | 81 ++++++++++++------- .../log/writer/LocalFileLogWriter.java | 7 +- 3 files changed, 83 insertions(+), 56 deletions(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java index f5a0a133e9..ca869d9d9d 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java @@ -9,6 +9,7 @@ import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; import java.io.IOException; +import javax.annotation.concurrent.ThreadSafe; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -26,15 +27,16 @@ *

  • summary.log - Summary information for all data chunks * */ +@ThreadSafe public class SingleFileImportLogger extends AbstractImportLogger { protected static final String SUMMARY_LOG_FILE_NAME = "summary.log"; protected static final String SUCCESS_LOG_FILE_NAME = "success.json"; protected static final String FAILURE_LOG_FILE_NAME = "failure.json"; private static final Logger LOGGER = LoggerFactory.getLogger(SingleFileImportLogger.class); - private LogWriter summaryLogWriter; - private LogWriter successLogWriter; - private LogWriter failureLogWriter; + private volatile LogWriter summaryLogWriter; + private final LogWriter successLogWriter; + private final LogWriter failureLogWriter; /** * Creates a new instance of SingleFileImportLogger. Initializes the success and failure log @@ -133,10 +135,25 @@ protected void logError(String errorMessage, Exception exception) { * @throws IOException if an I/O error occurs while writing to the log */ private void logDataChunkSummary(ImportDataChunkStatus dataChunkStatus) throws IOException { + ensureSummaryLogWriterInitialized(); + synchronized (summaryLogWriter) { + writeImportDataChunkSummary(dataChunkStatus, summaryLogWriter); + } + } + + /** + * Ensures that the summary log writer is initialized in a thread-safe manner. + * + * @throws IOException if an error occurs while creating the log writer + */ + private void ensureSummaryLogWriterInitialized() throws IOException { if (summaryLogWriter == null) { - summaryLogWriter = createLogWriter(config.getLogDirectoryPath() + SUMMARY_LOG_FILE_NAME); + synchronized (this) { + if (summaryLogWriter == null) { + summaryLogWriter = createLogWriter(config.getLogDirectoryPath() + SUMMARY_LOG_FILE_NAME); + } + } } - writeImportDataChunkSummary(dataChunkStatus, summaryLogWriter); } /** @@ -162,17 +179,7 @@ private void writeImportDataChunkSummary( */ private LogWriter getLogWriterForTransactionBatch(ImportTransactionBatchResult batchResult) throws IOException { - String logFileName = batchResult.isSuccess() ? SUCCESS_LOG_FILE_NAME : FAILURE_LOG_FILE_NAME; - LogWriter logWriter = batchResult.isSuccess() ? successLogWriter : failureLogWriter; - if (logWriter == null) { - logWriter = createLogWriter(config.getLogDirectoryPath() + logFileName); - if (batchResult.isSuccess()) { - successLogWriter = logWriter; - } else { - failureLogWriter = logWriter; - } - } - return logWriter; + return batchResult.isSuccess() ? successLogWriter : failureLogWriter; } /** @@ -223,11 +230,11 @@ private void writeToLogWriter(LogWriter logWriter, JsonNode jsonNode) throws IOE * been completed. */ private void closeAllLogWriters() { - closeLogWriter(summaryLogWriter); - closeLogWriter(successLogWriter); - closeLogWriter(failureLogWriter); - summaryLogWriter = null; - successLogWriter = null; - failureLogWriter = null; + synchronized (this) { + closeLogWriter(summaryLogWriter); + closeLogWriter(successLogWriter); + closeLogWriter(failureLogWriter); + summaryLogWriter = null; + } } } diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java index 69deaf9445..6e64a269b6 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java @@ -10,8 +10,10 @@ import com.scalar.db.dataloader.core.dataimport.task.result.ImportTaskResult; import com.scalar.db.dataloader.core.dataimport.transactionbatch.ImportTransactionBatchResult; import java.io.IOException; -import java.util.HashMap; +import java.io.UncheckedIOException; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import javax.annotation.concurrent.ThreadSafe; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -31,6 +33,7 @@ * *

    Log writers are created on demand and closed when their corresponding data chunk is completed. */ +@ThreadSafe public class SplitByDataChunkImportLogger extends AbstractImportLogger { protected static final String SUMMARY_LOG_FILE_NAME_FORMAT = "data_chunk_%s_summary.json"; @@ -38,9 +41,9 @@ public class SplitByDataChunkImportLogger extends AbstractImportLogger { protected static final String SUCCESS_LOG_FILE_NAME_FORMAT = "data_chunk_%s_success.json"; private static final Logger LOGGER = LoggerFactory.getLogger(SplitByDataChunkImportLogger.class); - private final Map summaryLogWriters = new HashMap<>(); - private final Map successLogWriters = new HashMap<>(); - private final Map failureLogWriters = new HashMap<>(); + private final Map summaryLogWriters = new ConcurrentHashMap<>(); + private final Map successLogWriters = new ConcurrentHashMap<>(); + private final Map failureLogWriters = new ConcurrentHashMap<>(); /** * Creates a new instance of SplitByDataChunkImportLogger. @@ -65,7 +68,7 @@ public void onTaskComplete(ImportTaskResult taskResult) { try { writeImportTaskResultDetailToLogs(taskResult); } catch (IOException e) { - LOGGER.error("Failed to write success/failure logs"); + LOGGER.error("Failed to write success/failure logs", e); } } @@ -78,31 +81,39 @@ public void onTaskComplete(ImportTaskResult taskResult) { */ private void writeImportTaskResultDetailToLogs(ImportTaskResult importTaskResult) throws IOException { - JsonNode jsonNode; for (ImportTargetResult target : importTaskResult.getTargets()) { - if (config.isLogSuccessRecords() - && target.getStatus().equals(ImportTargetResultStatus.SAVED)) { - jsonNode = OBJECT_MAPPER.valueToTree(target); - synchronized (successLogWriters) { - LogWriter successLogWriter = - initializeLogWriterIfNeeded(LogFileType.SUCCESS, importTaskResult.getDataChunkId()); - successLogWriter.write(jsonNode); - successLogWriter.flush(); - } - } - if (config.isLogRawSourceRecords() - && !target.getStatus().equals(ImportTargetResultStatus.SAVED)) { - jsonNode = OBJECT_MAPPER.valueToTree(target); - synchronized (failureLogWriters) { - LogWriter failureLogWriter = - initializeLogWriterIfNeeded(LogFileType.FAILURE, importTaskResult.getDataChunkId()); - failureLogWriter.write(jsonNode); - failureLogWriter.flush(); - } + ImportTargetResultStatus status = target.getStatus(); + if (status.equals(ImportTargetResultStatus.SAVED) && config.isLogSuccessRecords()) { + writeLog(target, LogFileType.SUCCESS, importTaskResult.getDataChunkId()); + } else if (!status.equals(ImportTargetResultStatus.SAVED) && config.isLogRawSourceRecords()) { + writeLog(target, LogFileType.FAILURE, importTaskResult.getDataChunkId()); } } } + /** + * Serializes the given {@link ImportTargetResult} to JSON and writes it to a log file + * corresponding to the provided {@link LogFileType} and data chunk ID. + * + *

    This method ensures thread-safe access to the underlying {@link LogWriter} by synchronizing + * on the writer instance. It is safe to call concurrently from multiple threads handling the same + * or different data chunks. + * + * @param target the result of processing a single import target to be logged + * @param logFileType the type of log file to write to (e.g., SUCCESS or FAILURE) + * @param dataChunkId the ID of the data chunk associated with the log entry + * @throws IOException if writing or flushing the log fails + */ + private void writeLog(ImportTargetResult target, LogFileType logFileType, int dataChunkId) + throws IOException { + JsonNode jsonNode = OBJECT_MAPPER.valueToTree(target); + LogWriter writer = initializeLogWriterIfNeeded(logFileType, dataChunkId); + synchronized (writer) { + writer.write(jsonNode); + writer.flush(); + } + } + /** * Called to add or update the status of a data chunk. This implementation does nothing as the * status is only logged when the data chunk is completed. @@ -245,11 +256,19 @@ private String getLogFilePath(long batchId, LogFileType logFileType) { private LogWriter initializeLogWriterIfNeeded(LogFileType logFileType, int dataChunkId) throws IOException { Map logWriters = getLogWriters(logFileType); - if (!logWriters.containsKey(dataChunkId)) { - LogWriter logWriter = createLogWriter(logFileType, dataChunkId); - logWriters.put(dataChunkId, logWriter); + try { + return logWriters.computeIfAbsent( + dataChunkId, + id -> { + try { + return createLogWriter(logFileType, id); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + }); + } catch (UncheckedIOException e) { + throw e.getCause(); } - return logWriters.get(dataChunkId); } /** @@ -272,7 +291,7 @@ private LogWriter createLogWriter(LogFileType logFileType, int dataChunkId) thro * @return the map of log writers for the specified type */ private Map getLogWriters(LogFileType logFileType) { - Map logWriterMap = null; + Map logWriterMap; switch (logFileType) { case SUCCESS: logWriterMap = successLogWriters; @@ -283,6 +302,8 @@ private Map getLogWriters(LogFileType logFileType) { case SUMMARY: logWriterMap = summaryLogWriters; break; + default: + throw new AssertionError(); } return logWriterMap; } diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java index 323c02661b..1c9725dc3c 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java @@ -9,6 +9,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardOpenOption; +import javax.annotation.Nullable; /** * An implementation of {@link LogWriter} that writes log entries to a local file. This class writes @@ -50,13 +51,11 @@ public LocalFileLogWriter(String filePath, ImportLoggerConfig importLoggerConfig * @throws IOException if an I/O error occurs while writing the record */ @Override - public void write(JsonNode sourceRecord) throws IOException { + public void write(@Nullable JsonNode sourceRecord) throws IOException { if (sourceRecord == null) { return; } - synchronized (logWriter) { - objectMapper.writeValue(logWriter, sourceRecord); - } + objectMapper.writeValue(logWriter, sourceRecord); } /** From 1afbc21ab306324bced7a932b161d8e3935939d1 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Mon, 21 Apr 2025 17:40:31 +0530 Subject: [PATCH 07/30] Renamed parameters --- .../core/dataimport/log/ImportLoggerConfig.java | 4 ++-- .../dataimport/log/SingleFileImportLoggerTest.java | 12 ++++++------ .../log/SplitByDataChunkImportLoggerTest.java | 12 ++++++------ .../log/writer/DefaultLogWriterFactoryTest.java | 4 ++-- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java index 9e0033fb26..f33c4ba188 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/ImportLoggerConfig.java @@ -21,13 +21,13 @@ public class ImportLoggerConfig { * Whether to log records that were successfully imported. If true, successful import operations * will be logged to success log files. */ - boolean logSuccessRecords; + boolean isLogSuccessRecords; /** * Whether to log raw source records that failed to be imported. If true, failed import operations * will be logged to failure log files. */ - boolean logRawSourceRecords; + boolean isLogRawSourceRecords; /** * Whether to format the logs with pretty printing. If true, the JSON logs will be formatted with diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java index e2d17dfa4f..69b9454477 100644 --- a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLoggerTest.java @@ -42,8 +42,8 @@ void setUp() { ImportLoggerConfig importLoggerConfig = ImportLoggerConfig.builder() .prettyPrint(false) - .logSuccessRecords(false) - .logRawSourceRecords(false) + .isLogSuccessRecords(false) + .isLogRawSourceRecords(false) .logDirectoryPath("path") .build(); logWriterFactory = new DefaultLogWriterFactory(importLoggerConfig); @@ -84,8 +84,8 @@ private void testTransactionBatchCompleted(boolean success, boolean logSuccessRe ImportLoggerConfig config = ImportLoggerConfig.builder() .logDirectoryPath(tempDir.toString() + "/") - .logRawSourceRecords(true) - .logSuccessRecords(logSuccessRecords) + .isLogRawSourceRecords(true) + .isLogSuccessRecords(logSuccessRecords) .build(); SingleFileImportLogger importLogger = new SingleFileImportLogger(config, logWriterFactory); @@ -199,8 +199,8 @@ private void testDataChunkCompleted(boolean hasErrors) throws IOException { ImportLoggerConfig config = ImportLoggerConfig.builder() .logDirectoryPath(tempDir.toString() + "/") - .logRawSourceRecords(true) - .logSuccessRecords(true) + .isLogRawSourceRecords(true) + .isLogSuccessRecords(true) .build(); SingleFileImportLogger importLogger = new SingleFileImportLogger(config, logWriterFactory); diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java index 04d9906641..de7ee49be3 100644 --- a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLoggerTest.java @@ -38,8 +38,8 @@ void setUp() { ImportLoggerConfig importLoggerConfig = ImportLoggerConfig.builder() .prettyPrint(false) - .logSuccessRecords(false) - .logRawSourceRecords(false) + .isLogSuccessRecords(false) + .isLogRawSourceRecords(false) .logDirectoryPath("path") .build(); logWriterFactory = new DefaultLogWriterFactory(importLoggerConfig); @@ -69,8 +69,8 @@ private void testTransactionBatchCompleted(boolean success, boolean logSuccessRe ImportLoggerConfig config = ImportLoggerConfig.builder() .logDirectoryPath(tempDir.toString() + "/") - .logRawSourceRecords(true) - .logSuccessRecords(logSuccessRecords) + .isLogRawSourceRecords(true) + .isLogSuccessRecords(logSuccessRecords) .build(); SplitByDataChunkImportLogger importLogger = new SplitByDataChunkImportLogger(config, logWriterFactory); @@ -175,8 +175,8 @@ private void testDataChunkCompleted(String logFilePattern, boolean hasErrors) th ImportLoggerConfig config = ImportLoggerConfig.builder() .logDirectoryPath(tempDir.toString() + "/") - .logRawSourceRecords(true) - .logSuccessRecords(true) + .isLogRawSourceRecords(true) + .isLogSuccessRecords(true) .build(); SplitByDataChunkImportLogger importLogger = new SplitByDataChunkImportLogger(config, logWriterFactory); diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java index 3b99510a7e..28c31e5c03 100644 --- a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataimport/log/writer/DefaultLogWriterFactoryTest.java @@ -28,8 +28,8 @@ void createLogWriter_withValidLocalLogFilePath_shouldReturnLocalFileLogWriterObj new DefaultLogWriterFactory( ImportLoggerConfig.builder() .prettyPrint(false) - .logSuccessRecords(false) - .logRawSourceRecords(false) + .isLogSuccessRecords(false) + .isLogRawSourceRecords(false) .logDirectoryPath("path") .build()); LogWriter logWriter = defaultLogWriterFactory.createLogWriter(filePath); From 8c5114d19095edff00800a1ffa9f81950d9a14c0 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Mon, 21 Apr 2025 17:49:36 +0530 Subject: [PATCH 08/30] logging changes --- .../core/dataimport/log/SplitByDataChunkImportLogger.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java index 6e64a269b6..2024beeb82 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java @@ -68,7 +68,7 @@ public void onTaskComplete(ImportTaskResult taskResult) { try { writeImportTaskResultDetailToLogs(taskResult); } catch (IOException e) { - LOGGER.error("Failed to write success/failure logs", e); + logError("Failed to write success/failure logs", e); } } @@ -136,7 +136,7 @@ public void onDataChunkCompleted(ImportDataChunkStatus dataChunkStatus) { // Close the split log writers per data chunk if they exist for this data chunk id closeLogWritersForDataChunk(dataChunkStatus.getDataChunkId()); } catch (IOException e) { - LOGGER.error("Failed to log the data chunk summary", e); + logError("Failed to log the data chunk summary", e); } } @@ -163,7 +163,7 @@ protected void logTransactionBatch(ImportTransactionBatchResult batchResult) { logWriter.flush(); } } catch (IOException e) { - LOGGER.error("Failed to write a transaction batch record to a split mode log file", e); + logError("Failed to write a transaction batch record to a split mode log file", e); } } From ffab395dd6d95c14f8c5a868dbf83a1bced3b928 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Tue, 22 Apr 2025 08:51:21 +0530 Subject: [PATCH 09/30] removed repeated code --- .../dataloader/core/dataimport/log/AbstractImportLogger.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java index 4a6121a97b..74d2537685 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/AbstractImportLogger.java @@ -127,8 +127,7 @@ protected JsonNode createFilteredTransactionBatchLogJsonNode( ImportTaskResult.builder() .rowNumber(taskResult.getRowNumber()) .targets(targetResults) - .dataChunkId(taskResult.getDataChunkId()) - .rowNumber(taskResult.getRowNumber()); + .dataChunkId(taskResult.getDataChunkId()); // Only add the raw record if the configuration is set to log raw source data if (config.isLogRawSourceRecords()) { From 6dd213e9e9c33e94ab25f474b2350f7bdf0c5f90 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Wed, 23 Apr 2025 14:23:15 +0530 Subject: [PATCH 10/30] Added excetpion throw --- .../dataloader/core/dataimport/log/SingleFileImportLogger.java | 1 + .../core/dataimport/log/SplitByDataChunkImportLogger.java | 1 + 2 files changed, 2 insertions(+) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java index ca869d9d9d..502227b1ec 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java @@ -125,6 +125,7 @@ protected void logTransactionBatch(ImportTransactionBatchResult batchResult) { @Override protected void logError(String errorMessage, Exception exception) { LOGGER.error(errorMessage, exception); + throw new RuntimeException(errorMessage, exception); } /** diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java index 2024beeb82..1ab63a2520 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java @@ -176,6 +176,7 @@ protected void logTransactionBatch(ImportTransactionBatchResult batchResult) { @Override protected void logError(String errorMessage, Exception exception) { LOGGER.error(errorMessage, exception); + throw new RuntimeException(errorMessage, exception); } /** From 65421779019b2031488223320e048ee55c61bf7e Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Fri, 25 Apr 2025 12:15:24 +0530 Subject: [PATCH 11/30] Synchronisation changes --- .../log/SingleFileImportLogger.java | 20 +++++-------------- .../log/SplitByDataChunkImportLogger.java | 14 ++++--------- .../log/writer/LocalFileLogWriter.java | 6 +++--- 3 files changed, 12 insertions(+), 28 deletions(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java index 502227b1ec..15a63bbf9f 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java @@ -34,7 +34,7 @@ public class SingleFileImportLogger extends AbstractImportLogger { protected static final String SUCCESS_LOG_FILE_NAME = "success.json"; protected static final String FAILURE_LOG_FILE_NAME = "failure.json"; private static final Logger LOGGER = LoggerFactory.getLogger(SingleFileImportLogger.class); - private volatile LogWriter summaryLogWriter; + private LogWriter summaryLogWriter; private final LogWriter successLogWriter; private final LogWriter failureLogWriter; @@ -137,9 +137,7 @@ protected void logError(String errorMessage, Exception exception) { */ private void logDataChunkSummary(ImportDataChunkStatus dataChunkStatus) throws IOException { ensureSummaryLogWriterInitialized(); - synchronized (summaryLogWriter) { - writeImportDataChunkSummary(dataChunkStatus, summaryLogWriter); - } + writeImportDataChunkSummary(dataChunkStatus, summaryLogWriter); } /** @@ -197,19 +195,12 @@ private void writeImportTaskResultDetailToLogs(ImportTaskResult importTaskResult for (ImportTargetResult target : importTaskResult.getTargets()) { if (config.isLogSuccessRecords() && target.getStatus().equals(ImportTargetResultStatus.SAVED)) { - synchronized (successLogWriter) { - jsonNode = OBJECT_MAPPER.valueToTree(target); - successLogWriter.write(jsonNode); - successLogWriter.flush(); - } + + writeToLogWriter(successLogWriter, OBJECT_MAPPER.valueToTree(target)); } if (config.isLogRawSourceRecords() && !target.getStatus().equals(ImportTargetResultStatus.SAVED)) { - synchronized (failureLogWriter) { - jsonNode = OBJECT_MAPPER.valueToTree(target); - failureLogWriter.write(jsonNode); - failureLogWriter.flush(); - } + writeToLogWriter(failureLogWriter, OBJECT_MAPPER.valueToTree(target)); } } } @@ -223,7 +214,6 @@ private void writeImportTaskResultDetailToLogs(ImportTaskResult importTaskResult */ private void writeToLogWriter(LogWriter logWriter, JsonNode jsonNode) throws IOException { logWriter.write(jsonNode); - logWriter.flush(); } /** diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java index 1ab63a2520..ab702babbf 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SplitByDataChunkImportLogger.java @@ -108,10 +108,7 @@ private void writeLog(ImportTargetResult target, LogFileType logFileType, int da throws IOException { JsonNode jsonNode = OBJECT_MAPPER.valueToTree(target); LogWriter writer = initializeLogWriterIfNeeded(logFileType, dataChunkId); - synchronized (writer) { - writer.write(jsonNode); - writer.flush(); - } + writer.write(jsonNode); } /** @@ -155,13 +152,10 @@ public void onAllDataChunksCompleted() { @Override protected void logTransactionBatch(ImportTransactionBatchResult batchResult) { LogFileType logFileType = batchResult.isSuccess() ? LogFileType.SUCCESS : LogFileType.FAILURE; - try (LogWriter logWriter = - initializeLogWriterIfNeeded(logFileType, batchResult.getDataChunkId())) { + try { + LogWriter logWriter = initializeLogWriterIfNeeded(logFileType, batchResult.getDataChunkId()); JsonNode jsonNode = createFilteredTransactionBatchLogJsonNode(batchResult); - synchronized (logWriter) { - logWriter.write(jsonNode); - logWriter.flush(); - } + logWriter.write(jsonNode); } catch (IOException e) { logError("Failed to write a transaction batch record to a split mode log file", e); } diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java index 1c9725dc3c..3689bd51d1 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/writer/LocalFileLogWriter.java @@ -51,7 +51,7 @@ public LocalFileLogWriter(String filePath, ImportLoggerConfig importLoggerConfig * @throws IOException if an I/O error occurs while writing the record */ @Override - public void write(@Nullable JsonNode sourceRecord) throws IOException { + public synchronized void write(@Nullable JsonNode sourceRecord) throws IOException { if (sourceRecord == null) { return; } @@ -64,7 +64,7 @@ public void write(@Nullable JsonNode sourceRecord) throws IOException { * @throws IOException if an I/O error occurs while flushing */ @Override - public void flush() throws IOException { + public synchronized void flush() throws IOException { logWriter.flush(); } @@ -75,7 +75,7 @@ public void flush() throws IOException { * @throws IOException if an I/O error occurs while closing the writer */ @Override - public void close() throws IOException { + public synchronized void close() throws IOException { if (logWriter.isClosed()) { return; } From 603e46ead1a70ca780a8f67f2f46bf4aa1b0f4e2 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Fri, 25 Apr 2025 12:31:20 +0530 Subject: [PATCH 12/30] Added volatile back to fix spotbugs issue --- .../dataloader/core/dataimport/log/SingleFileImportLogger.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java index 15a63bbf9f..7aae529bb5 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java @@ -34,7 +34,7 @@ public class SingleFileImportLogger extends AbstractImportLogger { protected static final String SUCCESS_LOG_FILE_NAME = "success.json"; protected static final String FAILURE_LOG_FILE_NAME = "failure.json"; private static final Logger LOGGER = LoggerFactory.getLogger(SingleFileImportLogger.class); - private LogWriter summaryLogWriter; + private volatile LogWriter summaryLogWriter; private final LogWriter successLogWriter; private final LogWriter failureLogWriter; From eaf9d88c224ce4830dbeb896b21061e4c060ee85 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Fri, 25 Apr 2025 12:44:21 +0530 Subject: [PATCH 13/30] Removed unused variable --- .../dataloader/core/dataimport/log/SingleFileImportLogger.java | 1 - 1 file changed, 1 deletion(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java index 7aae529bb5..151c329642 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/log/SingleFileImportLogger.java @@ -191,7 +191,6 @@ private LogWriter getLogWriterForTransactionBatch(ImportTransactionBatchResult b */ private void writeImportTaskResultDetailToLogs(ImportTaskResult importTaskResult) throws IOException { - JsonNode jsonNode; for (ImportTargetResult target : importTaskResult.getTargets()) { if (config.isLogSuccessRecords() && target.getStatus().equals(ImportTargetResultStatus.SAVED)) { From 0a2518a14bb46f14e8e9566506a01f26a03f3497 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Tue, 29 Apr 2025 10:56:02 +0530 Subject: [PATCH 14/30] Changes --- .../com/scalar/db/common/error/CoreError.java | 28 ++++++++++++++ .../cli/command/dataexport/ExportCommand.java | 4 +- .../dataloader/cli/util/DirectoryUtils.java | 23 +++++++++++- .../db/dataloader/cli/util/FileUtils.java | 37 +++++++++++++++++++ .../cli/util/InvalidFilePathException.java | 12 ++++++ .../db/dataloader/cli/util/ScalarDbUtils.java | 6 +++ .../cli/util/DirectoryUtilsTest.java | 16 ++++---- .../db/dataloader/cli/util/FileUtilsTest.java | 32 ++++++++++++++++ 8 files changed, 146 insertions(+), 12 deletions(-) mode change 100644 => 100755 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/DirectoryUtils.java create mode 100755 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java create mode 100755 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/InvalidFilePathException.java create mode 100644 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/ScalarDbUtils.java mode change 100644 => 100755 data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java create mode 100755 data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/FileUtilsTest.java diff --git a/core/src/main/java/com/scalar/db/common/error/CoreError.java b/core/src/main/java/com/scalar/db/common/error/CoreError.java index e3ec9a8032..db8da92805 100644 --- a/core/src/main/java/com/scalar/db/common/error/CoreError.java +++ b/core/src/main/java/com/scalar/db/common/error/CoreError.java @@ -848,6 +848,34 @@ public enum CoreError implements ScalarDbError { Category.USER_ERROR, "0186", "The CSV row: %s does not match header: %s.", "", ""), DATA_LOADER_JSON_CONTENT_START_ERROR( Category.USER_ERROR, "0187", "Expected JSON file content to be an array", "", ""), + DATA_LOADER_IMPORT_TARGET_MISSING( + Category.USER_ERROR, + "0188", + "Missing option: either '--namespace' and'--table' or '--control-file' options must be specified.", + "", + ""), + DATA_LOADER_MISSING_IMPORT_FILE( + Category.USER_ERROR, + "0189", + "The file '%s' specified by the argument '%s' does not exist.", + "", + ""), + DATA_LOADER_LOG_DIRECTORY_WRITE_ACCESS_DENIED( + Category.USER_ERROR, "0190", "Cannot write to the log directory: %s", "", ""), + DATA_LOADER_LOG_DIRECTORY_CREATION_FAILED( + Category.USER_ERROR, "0191", "Failed to create the log directory: %s", "", ""), + DATA_LOADER_INVALID_CONTROL_FILE( + Category.USER_ERROR, "0192", "Failed to parse the control file: %s", "", ""), + DATA_LOADER_DIRECTORY_WRITE_ACCESS( + Category.USER_ERROR, + "0193", + "No permission to create or write files in the directory: %s", + "", + ""), + DATA_LOADER_DIRECTORY_CREATION_FAILED( + Category.USER_ERROR, "0194", "Failed to create the directory: %s", "", ""), + DATA_LOADER_PATH_IS_NOT_A_DIRECTORY( + Category.USER_ERROR, "0195", "Path exists but is not a directory: %s", "", ""), // // Errors for the concurrency error category diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java index 873fad3c22..4c88be4594 100644 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java @@ -49,9 +49,9 @@ private void validateOutputDirectory(@Nullable String path) private void validateDirectory(String directoryPath) throws DirectoryValidationException { // If the directory path is null or empty, use the current working directory if (directoryPath == null || directoryPath.isEmpty()) { - DirectoryUtils.validateTargetDirectory(DirectoryUtils.getCurrentWorkingDirectory()); + DirectoryUtils.validateOrCreateTargetDirectory(DirectoryUtils.getCurrentWorkingDirectory()); } else { - DirectoryUtils.validateTargetDirectory(directoryPath); + DirectoryUtils.validateOrCreateTargetDirectory(directoryPath); } } diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/DirectoryUtils.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/DirectoryUtils.java old mode 100644 new mode 100755 index decb0f9ae7..a9a14ca4d7 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/DirectoryUtils.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/DirectoryUtils.java @@ -15,6 +15,22 @@ private DirectoryUtils() { // restrict instantiation } + /** + * Validates the current working directory. Ensures that it is writable. + * + * @throws DirectoryValidationException if the current working directory is not writable + */ + public static void validateWorkingDirectory() throws DirectoryValidationException { + Path workingDirectoryPath = Paths.get(System.getProperty("user.dir")); + + // Check if the current working directory is writable + if (!Files.isWritable(workingDirectoryPath)) { + throw new DirectoryValidationException( + CoreError.DATA_LOADER_DIRECTORY_WRITE_ACCESS.buildMessage( + workingDirectoryPath.toAbsolutePath())); + } + } + /** * Validates the provided directory path. Ensures that the directory exists and is writable. If * the directory doesn't exist, a creation attempt is made. @@ -22,7 +38,7 @@ private DirectoryUtils() { * @param directoryPath the directory path to validate * @throws DirectoryValidationException if the directory is not writable or cannot be created */ - public static void validateTargetDirectory(String directoryPath) + public static void validateOrCreateTargetDirectory(String directoryPath) throws DirectoryValidationException { if (StringUtils.isBlank(directoryPath)) { throw new IllegalArgumentException( @@ -32,7 +48,10 @@ public static void validateTargetDirectory(String directoryPath) Path path = Paths.get(directoryPath); if (Files.exists(path)) { - // Check if the provided directory is writable + if (!Files.isDirectory(path)) { + throw new DirectoryValidationException( + CoreError.DATA_LOADER_PATH_IS_NOT_A_DIRECTORY.buildMessage(path)); + } if (!Files.isWritable(path)) { throw new DirectoryValidationException( CoreError.DATA_LOADER_DIRECTORY_WRITE_ACCESS_NOT_ALLOWED.buildMessage( diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java new file mode 100755 index 0000000000..2a9e9ceec5 --- /dev/null +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java @@ -0,0 +1,37 @@ +package com.scalar.db.dataloader.cli.util; + +import java.nio.file.Path; +import java.nio.file.Paths; +import org.apache.commons.lang3.StringUtils; + +public class FileUtils { + + /** + * Validates the provided file path. + * + * @param filePath the file path to validate + * @throws InvalidFilePathException if the file path is invalid + */ + public static void validateFilePath(String filePath) throws InvalidFilePathException { + if (StringUtils.isBlank(filePath)) { + throw new IllegalArgumentException("File path must not be blank."); + } + Path pathToCheck = Paths.get(filePath); + + if (!pathToCheck.isAbsolute()) { + // If the path is not absolute, it's either a file name or a relative path + Path currentDirectory = Paths.get("").toAbsolutePath(); + Path fileInCurrentDirectory = currentDirectory.resolve(pathToCheck); + + if (!fileInCurrentDirectory.toFile().exists()) { + throw new InvalidFilePathException("File not found: " + pathToCheck); + } + return; + } + + // It's an absolute path + if (!pathToCheck.toFile().exists()) { + throw new InvalidFilePathException("File not found: " + pathToCheck); + } + } +} diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/InvalidFilePathException.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/InvalidFilePathException.java new file mode 100755 index 0000000000..d9e018286d --- /dev/null +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/InvalidFilePathException.java @@ -0,0 +1,12 @@ +package com.scalar.db.dataloader.cli.util; + +public class InvalidFilePathException extends Exception { + + public InvalidFilePathException(String message) { + super(message); + } + + public InvalidFilePathException(String message, Throwable cause) { + super(message, cause); + } +} diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/ScalarDbUtils.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/ScalarDbUtils.java new file mode 100644 index 0000000000..34314f6dc1 --- /dev/null +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/ScalarDbUtils.java @@ -0,0 +1,6 @@ +package com.scalar.db.dataloader.cli.util; + +/** The utility class for ScalarDB */ +public class ScalarDbUtils { + public static final String SCALAR_DB_TRANSACTION_MANAGER_CLUSTER = "cluster"; +} diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java old mode 100644 new mode 100755 index 60c0eafa60..ad4db52f0d --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java @@ -30,26 +30,26 @@ public void cleanup() throws IOException { @Test void validateTargetDirectory_ValidDirectory_NoExceptionThrown() throws DirectoryValidationException { - DirectoryUtils.validateTargetDirectory(tempDir.toString()); + DirectoryUtils.validateOrCreateTargetDirectory(tempDir.toString()); } @Test - void validateTargetDirectory_DirectoryDoesNotExist_CreatesDirectory() + void validateOrCreateTargetDirectory_DirectoryDoesNotExist_CreatesDirectory() throws DirectoryValidationException { Path newDirectory = Paths.get(tempDir.toString(), "newDir"); - DirectoryUtils.validateTargetDirectory(newDirectory.toString()); + DirectoryUtils.validateOrCreateTargetDirectory(newDirectory.toString()); assertTrue(Files.exists(newDirectory)); } @Test - void validateTargetDirectory_DirectoryNotWritable_ThrowsException() throws IOException { + void validateOrCreateTargetDirectory_DirectoryNotWritable_ThrowsException() throws IOException { Path readOnlyDirectory = Files.createDirectory(Paths.get(tempDir.toString(), "readOnlyDir")); readOnlyDirectory.toFile().setWritable(false); assertThrows( DirectoryValidationException.class, () -> { - DirectoryUtils.validateTargetDirectory(readOnlyDirectory.toString()); + DirectoryUtils.validateOrCreateTargetDirectory(readOnlyDirectory.toString()); }); } @@ -58,16 +58,16 @@ void validateTargetDirectory_NullDirectory_ThrowsException() { assertThrows( IllegalArgumentException.class, () -> { - DirectoryUtils.validateTargetDirectory(null); + DirectoryUtils.validateOrCreateTargetDirectory(null); }); } @Test - void validateTargetDirectory_EmptyDirectory_ThrowsException() { + void validateOrCreateTargetDirectory_EmptyDirectory_ThrowsException() { assertThrows( IllegalArgumentException.class, () -> { - DirectoryUtils.validateTargetDirectory(""); + DirectoryUtils.validateOrCreateTargetDirectory(""); }); } diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/FileUtilsTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/FileUtilsTest.java new file mode 100755 index 0000000000..c31c38c1ff --- /dev/null +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/FileUtilsTest.java @@ -0,0 +1,32 @@ +package com.scalar.db.dataloader.cli.util; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.file.Paths; +import org.junit.jupiter.api.Test; + +public class FileUtilsTest { + + private static final String currentPath = Paths.get("").toAbsolutePath().toString(); + + @Test + void validateFilePath_withValidFilePath_shouldNotThrowException() + throws InvalidFilePathException { + // Test and confirm no exception is thrown when a valid path is provided + FileUtils.validateFilePath(currentPath); + } + + @Test + void validateFilePath_withInvalidFilePath_shouldThrowException() { + assertThatThrownBy(() -> FileUtils.validateFilePath(currentPath + "/demo")) + .isInstanceOf(InvalidFilePathException.class) + .hasMessage("File not found: " + currentPath + "/demo"); + } + + @Test + void validateFilePath_withBlankFilePath_shouldThrowException() { + assertThatThrownBy(() -> FileUtils.validateFilePath("")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("File path must not be blank."); + } +} From 378effb9df4ed42bedfa7ee1d8724254cdf9d8bf Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Tue, 29 Apr 2025 14:28:10 +0530 Subject: [PATCH 15/30] Initial commit --- .../cli/command/dataexport/ExportCommand.java | 206 ++++++++++++++---- .../dataexport/ExportCommandOptions.java | 141 +++++++++++- .../MultiColumnKeyValueConverter.java | 65 ++++++ .../dataexport/ScanOrderingConverter.java | 25 +++ .../SingleColumnKeyValueConverter.java | 43 ++++ .../command/dataexport/ExportCommandTest.java | 130 +++-------- .../MultiColumnKeyValueConverterTest.java | 36 +++ .../dataexport/ScanOrderingConverterTest.java | 42 ++++ .../SingleColumnKeyValueConverterTest.java | 62 ++++++ .../db/dataloader/core/util/KeyUtils.java | 22 ++ .../db/dataloader/core/util/KeyUtilsTest.java | 19 ++ 11 files changed, 656 insertions(+), 135 deletions(-) mode change 100644 => 100755 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java mode change 100644 => 100755 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java create mode 100755 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java create mode 100755 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java create mode 100755 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java mode change 100644 => 100755 data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java create mode 100755 data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java create mode 100755 data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java create mode 100755 data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverterTest.java diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java old mode 100644 new mode 100755 index 4c88be4594..7db57909dd --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java @@ -1,70 +1,202 @@ package com.scalar.db.dataloader.cli.command.dataexport; -import com.scalar.db.common.error.CoreError; +import static java.nio.file.StandardOpenOption.APPEND; +import static java.nio.file.StandardOpenOption.CREATE; + +import com.scalar.db.api.DistributedStorage; +import com.scalar.db.api.TableMetadata; import com.scalar.db.dataloader.cli.exception.DirectoryValidationException; -import com.scalar.db.dataloader.cli.exception.InvalidFileExtensionException; import com.scalar.db.dataloader.cli.util.DirectoryUtils; -import java.io.File; -import java.util.Arrays; +import com.scalar.db.dataloader.cli.util.FileUtils; +import com.scalar.db.dataloader.cli.util.InvalidFilePathException; +import com.scalar.db.dataloader.core.ColumnKeyValue; +import com.scalar.db.dataloader.core.FileFormat; +import com.scalar.db.dataloader.core.ScanRange; +import com.scalar.db.dataloader.core.dataexport.CsvExportManager; +import com.scalar.db.dataloader.core.dataexport.ExportManager; +import com.scalar.db.dataloader.core.dataexport.ExportOptions; +import com.scalar.db.dataloader.core.dataexport.JsonExportManager; +import com.scalar.db.dataloader.core.dataexport.JsonLineExportManager; +import com.scalar.db.dataloader.core.dataexport.producer.ProducerTaskFactory; +import com.scalar.db.dataloader.core.dataimport.dao.ScalarDBDao; +import com.scalar.db.dataloader.core.exception.ColumnParsingException; +import com.scalar.db.dataloader.core.exception.KeyParsingException; +import com.scalar.db.dataloader.core.tablemetadata.TableMetadataException; +import com.scalar.db.dataloader.core.tablemetadata.TableMetadataService; +import com.scalar.db.dataloader.core.util.KeyUtils; +import com.scalar.db.io.Key; +import com.scalar.db.service.StorageFactory; +import java.io.BufferedWriter; +import java.nio.charset.Charset; +import java.nio.file.Files; +import java.nio.file.Paths; import java.util.List; +import java.util.Objects; import java.util.concurrent.Callable; -import javax.annotation.Nullable; -import org.apache.commons.io.FilenameUtils; import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import picocli.CommandLine; import picocli.CommandLine.Model.CommandSpec; import picocli.CommandLine.Spec; -@CommandLine.Command(name = "export", description = "Export data from a ScalarDB table") +@CommandLine.Command(name = "export", description = "export data from a ScalarDB table") public class ExportCommand extends ExportCommandOptions implements Callable { - private static final List ALLOWED_EXTENSIONS = Arrays.asList("csv", "json", "jsonl"); + private static final String EXPORT_FILE_NAME_FORMAT = "export_%s.%s_%s.%s"; + private static final Logger LOGGER = LoggerFactory.getLogger(ExportCommand.class); @Spec CommandSpec spec; @Override public Integer call() throws Exception { - validateOutputDirectory(outputFilePath); - return 0; - } + String scalarDbPropertiesFilePath = getScalarDbPropertiesFilePath(); + + try { + validateOutputDirectory(); + FileUtils.validateFilePath(scalarDbPropertiesFilePath); + + StorageFactory storageFactory = StorageFactory.create(scalarDbPropertiesFilePath); + TableMetadataService metaDataService = + new TableMetadataService(storageFactory.getStorageAdmin()); + ScalarDBDao scalarDBDao = new ScalarDBDao(); + + ExportManager exportManager = createExportManager(storageFactory, scalarDBDao, outputFormat); + + TableMetadata tableMetadata = metaDataService.getTableMetadata(namespace, table); - private void validateOutputDirectory(@Nullable String path) - throws DirectoryValidationException, InvalidFileExtensionException { - if (path == null || path.isEmpty()) { - // It is ok for the output file path to be null or empty as a default file name will be used - // if not provided - return; + Key partitionKey = + partitionKeyValue != null ? getKeysFromList(partitionKeyValue, tableMetadata) : null; + Key scanStartKey = + scanStartKeyValue != null + ? getKey(scanStartKeyValue, namespace, table, tableMetadata) + : null; + Key scanEndKey = + scanEndKeyValue != null ? getKey(scanEndKeyValue, namespace, table, tableMetadata) : null; + + ScanRange scanRange = + new ScanRange(scanStartKey, scanEndKey, scanStartInclusive, scanEndInclusive); + ExportOptions exportOptions = buildExportOptions(partitionKey, scanRange); + + String filePath = + getOutputAbsoluteFilePath( + outputDirectory, outputFileName, exportOptions.getOutputFileFormat()); + LOGGER.info("Exporting data to file: {}", filePath); + + try (BufferedWriter writer = + Files.newBufferedWriter(Paths.get(filePath), Charset.defaultCharset(), CREATE, APPEND)) { + exportManager.startExport(exportOptions, tableMetadata, writer); + } + + } catch (DirectoryValidationException e) { + LOGGER.error("Invalid output directory path: {}", outputDirectory); + return 1; + } catch (InvalidFilePathException e) { + LOGGER.error( + "The ScalarDB connection settings file path is invalid or the file is missing: {}", + scalarDbPropertiesFilePath); + return 1; + } catch (TableMetadataException e) { + LOGGER.error("Failed to retrieve table metadata: {}", e.getMessage()); + return 1; } + return 0; + } - File file = new File(path); + private String getScalarDbPropertiesFilePath() { + return Objects.equals(configFilePath, DEFAULT_CONFIG_FILE_NAME) + ? Paths.get("").toAbsolutePath().resolve(DEFAULT_CONFIG_FILE_NAME).toString() + : configFilePath; + } - if (file.isDirectory()) { - validateDirectory(path); + private void validateOutputDirectory() throws DirectoryValidationException { + if (StringUtils.isBlank(outputDirectory)) { + DirectoryUtils.validateWorkingDirectory(); } else { - validateFileExtension(file.getName()); - validateDirectory(file.getParent()); + DirectoryUtils.validateOrCreateTargetDirectory(outputDirectory); } } - private void validateDirectory(String directoryPath) throws DirectoryValidationException { - // If the directory path is null or empty, use the current working directory - if (directoryPath == null || directoryPath.isEmpty()) { - DirectoryUtils.validateOrCreateTargetDirectory(DirectoryUtils.getCurrentWorkingDirectory()); - } else { - DirectoryUtils.validateOrCreateTargetDirectory(directoryPath); + private ExportManager createExportManager( + StorageFactory storageFactory, ScalarDBDao scalarDBDao, FileFormat fileFormat) { + ProducerTaskFactory taskFactory = + new ProducerTaskFactory(delimiter, includeTransactionMetadata, prettyPrintJson); + DistributedStorage storage = storageFactory.getStorage(); + switch (fileFormat) { + case JSON: + return new JsonExportManager(storage, scalarDBDao, taskFactory); + case JSONL: + return new JsonLineExportManager(storage, scalarDBDao, taskFactory); + case CSV: + return new CsvExportManager(storage, scalarDBDao, taskFactory); + default: + throw new AssertionError("Invalid file format" + fileFormat); } } - private void validateFileExtension(String filename) throws InvalidFileExtensionException { - String extension = FilenameUtils.getExtension(filename); - if (StringUtils.isBlank(extension)) { - throw new InvalidFileExtensionException( - CoreError.DATA_LOADER_MISSING_FILE_EXTENSION.buildMessage(filename)); + private ExportOptions buildExportOptions(Key partitionKey, ScanRange scanRange) { + ExportOptions.ExportOptionsBuilder builder = + ExportOptions.builder(namespace, table, partitionKey, outputFormat) + .sortOrders(sortOrders) + .excludeHeaderRow(excludeHeader) + .includeTransactionMetadata(includeTransactionMetadata) + .delimiter(delimiter) + .limit(limit) + .maxThreadCount(maxThreads) + .dataChunkSize(dataChunkSize) + .prettyPrintJson(prettyPrintJson) + .scanRange(scanRange); + + if (projectionColumns != null) { + builder.projectionColumns(projectionColumns); } - if (!ALLOWED_EXTENSIONS.contains(extension.toLowerCase())) { - throw new InvalidFileExtensionException( - CoreError.DATA_LOADER_INVALID_FILE_EXTENSION.buildMessage( - extension, String.join(", ", ALLOWED_EXTENSIONS))); + + return builder.build(); + } + + private String getOutputAbsoluteFilePath( + String outputDirectory, String outputFileName, FileFormat outputFormat) { + String fileName = + StringUtils.isBlank(outputFileName) + ? String.format( + EXPORT_FILE_NAME_FORMAT, + namespace, + table, + System.nanoTime(), + outputFormat.toString().toLowerCase()) + : outputFileName; + + if (StringUtils.isBlank(outputDirectory)) { + return Paths.get("").toAbsolutePath().resolve(fileName).toAbsolutePath().toString(); + } else { + return Paths.get(outputDirectory).resolve(fileName).toAbsolutePath().toString(); } } + + /** + * Convert ColumnKeyValue list to a key + * + * @param keyValueList key value list + * @param tableMetadata table metadata + * @return key + * @throws ColumnParsingException if any error occur during parsing column value + */ + private Key getKeysFromList(List keyValueList, TableMetadata tableMetadata) + throws ColumnParsingException { + return KeyUtils.parseMultipleKeyValues(keyValueList, tableMetadata); + } + + /** + * Convert ColumnKeyValue to a key + * + * @param keyValue key value + * @param tableMetadata table metadata + * @return key + * @throws KeyParsingException if any error occur during decoding key + */ + private Key getKey( + ColumnKeyValue keyValue, String namespace, String table, TableMetadata tableMetadata) + throws KeyParsingException { + return KeyUtils.parseKeyValue(keyValue, namespace, table, tableMetadata); + } } diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java old mode 100644 new mode 100755 index b5ac608f27..aa88c87343 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java @@ -1,14 +1,147 @@ package com.scalar.db.dataloader.cli.command.dataexport; +import com.scalar.db.api.Scan; +import com.scalar.db.dataloader.core.ColumnKeyValue; +import com.scalar.db.dataloader.core.FileFormat; +import java.util.ArrayList; +import java.util.List; import picocli.CommandLine; -/** A class to represent the command options for the export command. */ public class ExportCommandOptions { + protected static final String DEFAULT_CONFIG_FILE_NAME = "scalardb.properties"; + + @CommandLine.Option( + names = {"--config", "-c"}, + paramLabel = "", + description = "Path to the ScalarDB configuration file (default: scalardb.properties)", + defaultValue = DEFAULT_CONFIG_FILE_NAME) + protected String configFilePath; + + @CommandLine.Option( + names = {"--namespace", "-ns"}, + paramLabel = "", + required = true, + description = "ScalarDB namespace containing the table to export data from") + protected String namespace; + + @CommandLine.Option( + names = {"--table", "-t"}, + paramLabel = "", + required = true, + description = "Name of the ScalarDB table to export data from") + protected String table; + @CommandLine.Option( names = {"--output-file", "-o"}, - paramLabel = "", + paramLabel = "", description = - "Path and name of the output file for the exported data (default: .)") - protected String outputFilePath; + "Name of the output file for the exported data (default: .)") + protected String outputFileName; + + @CommandLine.Option( + names = {"--output-dir", "-d"}, + paramLabel = "", + description = + "Directory where the exported file should be saved (default: current directory)") + protected String outputDirectory; + + @CommandLine.Option( + names = {"--partition-key", "-pk"}, + paramLabel = "", + description = "ScalarDB partition key and value in the format 'key=value'", + converter = MultiColumnKeyValueConverter.class) + protected List partitionKeyValue; + + @CommandLine.Option( + names = {"--format", "-fmt"}, + paramLabel = "", + description = "Format of the exported data file (json, csv, tsv) (default: json)", + defaultValue = "json") + protected FileFormat outputFormat; + + @CommandLine.Option( + names = {"--include-metadata", "-m"}, + description = "Include transaction metadata in the exported data (default: false)", + defaultValue = "false") + protected boolean includeTransactionMetadata; + + @CommandLine.Option( + names = {"--max-threads", "-mt"}, + paramLabel = "", + description = + "Maximum number of threads to use for parallel processing (default: number of available processors)") + protected int maxThreads; + + @CommandLine.Option( + names = {"--start-key", "-sk"}, + paramLabel = "", + description = "Clustering key and value to mark the start of the scan (inclusive)", + converter = SingleColumnKeyValueConverter.class) + protected ColumnKeyValue scanStartKeyValue; + + @CommandLine.Option( + names = {"--start-inclusive", "-si"}, + description = "Make the start key inclusive (default: true)", + defaultValue = "true") + // TODO: test that -si false, works + protected boolean scanStartInclusive; + + @CommandLine.Option( + names = {"--end-key", "-ek"}, + paramLabel = "", + description = "Clustering key and value to mark the end of the scan (inclusive)", + converter = SingleColumnKeyValueConverter.class) + protected ColumnKeyValue scanEndKeyValue; + + @CommandLine.Option( + names = {"--end-inclusive", "-ei"}, + description = "Make the end key inclusive (default: true)", + defaultValue = "true") + protected boolean scanEndInclusive; + + @CommandLine.Option( + names = {"--sort-by", "-s"}, + paramLabel = "", + description = "Clustering key sorting order (asc, desc)", + converter = ScanOrderingConverter.class) + protected List sortOrders = new ArrayList<>(); + + @CommandLine.Option( + names = {"--projection", "-p"}, + paramLabel = "", + description = "Columns to include in the export (comma-separated)", + split = ",") + protected List projectionColumns; + + @CommandLine.Option( + names = {"--limit", "-l"}, + paramLabel = "", + description = "Maximum number of rows to export") + protected int limit; + + @CommandLine.Option( + names = {"--delimiter"}, + paramLabel = "", + defaultValue = ";", + description = "Delimiter character for CSV/TSV files (default: comma for CSV, tab for TSV)") + protected String delimiter; + + @CommandLine.Option( + names = {"--no-header", "-nh"}, + description = "Exclude header row in CSV/TSV files (default: false)", + defaultValue = "false") + protected boolean excludeHeader; + + @CommandLine.Option( + names = {"--pretty-print", "-pp"}, + description = "Pretty-print JSON output (default: false)", + defaultValue = "false") + protected boolean prettyPrintJson; + + @CommandLine.Option( + names = {"--data-chunk-size", "-dcs"}, + description = "Size of the data chunk to process in a single task (default: 200)", + defaultValue = "200") + protected int dataChunkSize; } diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java new file mode 100755 index 0000000000..955ae66966 --- /dev/null +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java @@ -0,0 +1,65 @@ +package com.scalar.db.dataloader.cli.command.dataexport; + +import com.scalar.db.dataloader.core.ColumnKeyValue; +import java.util.ArrayList; +import java.util.List; +import picocli.CommandLine; + +/** + * Converts a string representation of multiple key-value pairs into a list of {@link + * ColumnKeyValue} objects. + * + *

    The expected format for the input string is: {@code key1=value1,key2=value2,...}. Each + * key-value pair should be separated by a comma, and each pair must follow the "key=value" format. + * + *

    Example usage: + * + * 

    + *   MultiColumnKeyValueConverter converter = new MultiColumnKeyValueConverter();
+ *   List<ColumnKeyValue> result = converter.convert("name=John,age=30,city=New York");
+ *
    + */ +public class MultiColumnKeyValueConverter + implements CommandLine.ITypeConverter> { + + /** + * Converts a comma-separated string of key-value pairs into a list of {@link ColumnKeyValue} + * objects. + * + * @param keyValue the input string in the format {@code key1=value1,key2=value2,...} + * @return a list of {@link ColumnKeyValue} objects representing the parsed key-value pairs + * @throws IllegalArgumentException if the input is null, empty, or contains invalid formatting + */ + @Override + public List convert(String keyValue) { + if (keyValue == null || keyValue.trim().isEmpty()) { + throw new IllegalArgumentException("Key-value cannot be null or empty"); + } + + List columnKeyValueList = new ArrayList<>(); + String[] columnValues = keyValue.split(","); + + for (String columnValue : columnValues) { + columnKeyValueList.add(parseKeyValue(columnValue)); + } + + return columnKeyValueList; + } + + /** + * Parses a single key-value pair from a string in the format "key=value". + * + * @param keyValue the key-value string to parse + * @return a {@link ColumnKeyValue} object representing the parsed key-value pair + * @throws IllegalArgumentException if the input is not in the expected format + */ + private ColumnKeyValue parseKeyValue(String keyValue) { + String[] parts = keyValue.split("=", 2); + + if (parts.length != 2 || parts[0].trim().isEmpty() || parts[1].trim().isEmpty()) { + throw new IllegalArgumentException("Invalid key-value format: " + keyValue); + } + + return new ColumnKeyValue(parts[0].trim(), parts[1].trim()); + } +} diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java new file mode 100755 index 0000000000..fc70bfd07a --- /dev/null +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java @@ -0,0 +1,25 @@ +package com.scalar.db.dataloader.cli.command.dataexport; + +import com.scalar.db.api.Scan; +import java.util.ArrayList; +import java.util.List; +import picocli.CommandLine; + +public class ScanOrderingConverter implements CommandLine.ITypeConverter> { + @Override + public List convert(String value) { + String[] parts = value.split(","); + List scanOrderList = new ArrayList<>(); + for (String part : parts) { + String[] data = part.split("="); + + if (data.length != 2) { + throw new IllegalArgumentException("Invalid column order format: " + value); + } + String columnName = data[0].trim(); + Scan.Ordering.Order sortOrder = Scan.Ordering.Order.valueOf(data[1].trim().toUpperCase()); + scanOrderList.add(new Scan.Ordering(columnName, sortOrder)); + } + return scanOrderList; + } +} diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java new file mode 100755 index 0000000000..4a61958e74 --- /dev/null +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java @@ -0,0 +1,43 @@ +package com.scalar.db.dataloader.cli.command.dataexport; + +import com.scalar.db.dataloader.core.ColumnKeyValue; +import picocli.CommandLine; + +/** + * Converts a string representation of a key-value pair into a {@link ColumnKeyValue} object. The + * string format should be "key=value". + */ +public class SingleColumnKeyValueConverter implements CommandLine.ITypeConverter { + + /** + * Converts a string representation of a key-value pair into a {@link ColumnKeyValue} object. + * + * @param keyValue the string representation of the key-value pair in the format "key=value" + * @return a {@link ColumnKeyValue} object representing the key-value pair + * @throws IllegalArgumentException if the input string is not in the expected format + */ + @Override + public ColumnKeyValue convert(String keyValue) { + if (keyValue == null || keyValue.trim().isEmpty()) { + throw new IllegalArgumentException("Key-value cannot be null or empty"); + } + return parseKeyValue(keyValue); + } + + /** + * Parses a single key-value pair from a string in the format "key=value". + * + * @param keyValue the key-value string to parse + * @return a {@link ColumnKeyValue} object representing the parsed key-value pair + * @throws IllegalArgumentException if the input is not in the expected format + */ + private ColumnKeyValue parseKeyValue(String keyValue) { + String[] parts = keyValue.split("=", 2); + + if (parts.length != 2 || parts[0].trim().isEmpty() || parts[1].trim().isEmpty()) { + throw new IllegalArgumentException("Invalid key-value format: " + keyValue); + } + + return new ColumnKeyValue(parts[0].trim(), parts[1].trim()); + } +} diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java old mode 100644 new mode 100755 index 538de9f404..07a78b93b9 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java @@ -1,112 +1,54 @@ package com.scalar.db.dataloader.cli.command.dataexport; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertThrows; -import com.scalar.db.dataloader.cli.exception.InvalidFileExtensionException; -import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; +import com.scalar.db.dataloader.core.FileFormat; +import java.io.File; import java.nio.file.Paths; -import java.util.stream.Stream; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.io.TempDir; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import picocli.CommandLine; class ExportCommandTest { - private static final Logger LOGGER = LoggerFactory.getLogger(ExportCommandTest.class); - @TempDir Path tempDir; - - private ExportCommand exportCommand; - - @BeforeEach - void setUp() { - exportCommand = new ExportCommand(); - CommandLine cmd = new CommandLine(exportCommand); - exportCommand.spec = cmd.getCommandSpec(); - } - @AfterEach - public void cleanup() throws IOException { - cleanUpTempDir(); - } - - @Test - void call_WithValidOutputDirectory_ShouldReturnZero() throws Exception { - Path configFile = tempDir.resolve("config.properties"); - Files.createFile(configFile); - - Path outputDir = tempDir.resolve("output"); - Files.createDirectory(outputDir); - - exportCommand.outputFilePath = outputDir.toString(); - - assertEquals(0, exportCommand.call()); - } - - @Test - void call_WithInvalidOutputDirectory_ShouldThrowInvalidFileExtensionException() - throws IOException { - Path configFile = tempDir.resolve("config.properties"); - Files.createFile(configFile); - - Path outputDir = tempDir.resolve("output"); - outputDir.toFile().setWritable(false); - - exportCommand.outputFilePath = outputDir.toString(); - - assertThrows(InvalidFileExtensionException.class, () -> exportCommand.call()); - } - - @Test - void call_WithValidOutputFile_ShouldReturnZero() throws Exception { - Path configFile = tempDir.resolve("config.properties"); - Files.createFile(configFile); - - Path outputFile = tempDir.resolve("output.csv"); - - exportCommand.outputFilePath = outputFile.toString(); - - assertEquals(0, exportCommand.call()); + void removeFileIfCreated() { + // To remove generated file if it is present + String filePath = Paths.get("").toAbsolutePath() + "/sample.json"; + File file = new File(filePath); + if (file.exists()) { + file.deleteOnExit(); + } } @Test - void call_WithValidOutputFileInCurrentDirectory_ShouldReturnZero() throws Exception { - Path configFile = tempDir.resolve("config.properties"); - Files.createFile(configFile); - - Path outputFile = Paths.get("output.csv"); - - exportCommand.outputFilePath = outputFile.toString(); - - assertEquals(0, exportCommand.call()); + void call_withBlankScalarDBConfigurationFile_shouldThrowException() { + ExportCommand exportCommand = new ExportCommand(); + exportCommand.configFilePath = ""; + exportCommand.dataChunkSize = 100; + exportCommand.namespace = "scalar"; + exportCommand.table = "asset"; + exportCommand.outputDirectory = ""; + exportCommand.outputFileName = "sample.json"; + exportCommand.outputFormat = FileFormat.JSON; + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + exportCommand::call, + "Expected to throw FileNotFound exception as configuration path is invalid"); + Assertions.assertEquals("File path must not be blank.", thrown.getMessage()); } @Test - void call_WithValidOutputFileWithoutDirectory_ShouldReturnZero() throws Exception { - Path configFile = tempDir.resolve("config.properties"); - Files.createFile(configFile); - - exportCommand.outputFilePath = "output.csv"; - - assertEquals(0, exportCommand.call()); - } - - private void cleanUpTempDir() throws IOException { - try (Stream paths = Files.list(tempDir)) { - paths.forEach(this::deleteFile); - } - } - - private void deleteFile(Path file) { - try { - Files.deleteIfExists(file); - } catch (IOException e) { - LOGGER.error("Failed to delete file: {}", file, e); - } + void call_withInvalidScalarDBConfigurationFile_shouldReturnOne() throws Exception { + ExportCommand exportCommand = new ExportCommand(); + exportCommand.configFilePath = "scalardb.properties"; + exportCommand.dataChunkSize = 100; + exportCommand.namespace = "scalar"; + exportCommand.table = "asset"; + exportCommand.outputDirectory = ""; + exportCommand.outputFileName = "sample.json"; + exportCommand.outputFormat = FileFormat.JSON; + Assertions.assertEquals(1, exportCommand.call()); } } diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java new file mode 100755 index 0000000000..b7ee8ae36e --- /dev/null +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java @@ -0,0 +1,36 @@ +package com.scalar.db.dataloader.cli.command.dataexport; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.scalar.db.dataloader.core.ColumnKeyValue; +import java.util.Collections; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class MultiColumnKeyValueConverterTest { + + MultiColumnKeyValueConverter multiColumnKeyValueConverter = new MultiColumnKeyValueConverter(); + + @Test + public void convert_withInvalidValue_ShouldThrowError() { + String value = "id 15"; + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> multiColumnKeyValueConverter.convert(value), + "Expected to throw exception"); + Assertions.assertEquals("Invalid key-value format: id 15", thrown.getMessage()); + } + + @Test + public void convert_withValidValue_ShouldReturnColumnKeyValue() { + String value = "id=15"; + ColumnKeyValue expectedOrder = new ColumnKeyValue("id", "15"); + Assertions.assertEquals( + Collections.singletonList(expectedOrder).get(0).getColumnName(), + multiColumnKeyValueConverter.convert(value).get(0).getColumnName()); + Assertions.assertEquals( + Collections.singletonList(expectedOrder).get(0).getColumnValue(), + multiColumnKeyValueConverter.convert(value).get(0).getColumnValue()); + } +} diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java new file mode 100755 index 0000000000..206798514b --- /dev/null +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java @@ -0,0 +1,42 @@ +package com.scalar.db.dataloader.cli.command.dataexport; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.scalar.db.api.Scan; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class ScanOrderingConverterTest { + ScanOrderingConverter scanOrderingConverter = new ScanOrderingConverter(); + + @Test + public void callConvert_withInvalidValue_shouldThrowException() { + String value = "id ASC"; + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> scanOrderingConverter.convert(value), + "Expected to throw exception"); + Assertions.assertEquals("Invalid column order format: id ASC", thrown.getMessage()); + } + + @Test + public void callConvert_withValidValueAndOrderAscending_shouldReturnScanOrdering() { + String value = "id=ASC,age=DESC"; + List expectedOrder = new ArrayList<>(); + expectedOrder.add(new Scan.Ordering("id", Scan.Ordering.Order.ASC)); + expectedOrder.add(new Scan.Ordering("age", Scan.Ordering.Order.DESC)); + Assertions.assertEquals(expectedOrder, scanOrderingConverter.convert(value)); + } + + @Test + public void callConvert_withValidValueAndOrderDescending_shouldReturnScanOrdering() { + String value = "id=desc"; + List expectedOrder = + Collections.singletonList(new Scan.Ordering("id", Scan.Ordering.Order.DESC)); + Assertions.assertEquals(expectedOrder, scanOrderingConverter.convert(value)); + } +} diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverterTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverterTest.java new file mode 100755 index 0000000000..0cfc894383 --- /dev/null +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverterTest.java @@ -0,0 +1,62 @@ +package com.scalar.db.dataloader.cli.command.dataexport; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.scalar.db.dataloader.core.ColumnKeyValue; +import org.junit.jupiter.api.Test; + +class SingleColumnKeyValueConverterTest { + + private final SingleColumnKeyValueConverter converter = new SingleColumnKeyValueConverter(); + + @Test + void convert_ValidInput_ReturnsColumnKeyValue() { + String input = "name=John Doe"; + ColumnKeyValue expected = new ColumnKeyValue("name", "John Doe"); + ColumnKeyValue result = converter.convert(input); + assertEquals(expected.getColumnName(), result.getColumnName()); + assertEquals(expected.getColumnValue(), result.getColumnValue()); + } + + @Test + void convert_ValidInputWithExtraSpaces_ReturnsColumnKeyValue() { + String input = " age = 25 "; + ColumnKeyValue expected = new ColumnKeyValue("age", "25"); + ColumnKeyValue result = converter.convert(input); + assertEquals(expected.getColumnName(), result.getColumnName()); + assertEquals(expected.getColumnValue(), result.getColumnValue()); + } + + @Test + void convert_InvalidInputMissingValue_ThrowsIllegalArgumentException() { + String input = "name="; + assertThrows(IllegalArgumentException.class, () -> converter.convert(input)); + } + + @Test + void convert_InvalidInputMissingKey_ThrowsIllegalArgumentException() { + String input = "=John Doe"; + assertThrows(IllegalArgumentException.class, () -> converter.convert(input)); + } + + @Test + void convert_InvalidInputMissingEquals_ThrowsIllegalArgumentException() { + String input = "nameJohn Doe"; + assertThrows(IllegalArgumentException.class, () -> converter.convert(input)); + } + + @Test + void convert_ValidInputMultipleEquals_Returns() { + String input = "name=John=Doe"; + ColumnKeyValue expected = new ColumnKeyValue("name", "John=Doe"); + ColumnKeyValue result = converter.convert(input); + assertEquals(expected.getColumnName(), result.getColumnName()); + assertEquals(expected.getColumnValue(), result.getColumnValue()); + } + + @Test + void convert_NullValue_ThrowsIllegalArgumentException() { + assertThrows(IllegalArgumentException.class, () -> converter.convert(null)); + } +} diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/KeyUtils.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/KeyUtils.java index e3433a31b5..45f1ad22e2 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/KeyUtils.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/KeyUtils.java @@ -213,4 +213,26 @@ private static Optional createKeyFromSource( return Optional.empty(); } } + + /** + * Convert a keyValue, in the format of =, to a ScalarDB Key instance. + * + * @param keyValues A list of key values in the format of = + * @param tableMetadata Metadata for one ScalarDB table + * @return A new ScalarDB Key instance formatted by data type + * @throws ColumnParsingException if there is an error parsing the column + */ + public static Key parseMultipleKeyValues( + List keyValues, TableMetadata tableMetadata) throws ColumnParsingException { + Key.Builder builder = Key.newBuilder(); + for (ColumnKeyValue keyValue : keyValues) { + String columnName = keyValue.getColumnName(); + String value = keyValue.getColumnValue(); + DataType columnDataType = tableMetadata.getColumnDataType(columnName); + ColumnInfo columnInfo = ColumnInfo.builder().columnName(columnName).build(); + Column keyValueCol = ColumnUtils.createColumnFromValue(columnDataType, columnInfo, value); + builder.add(keyValueCol); + } + return builder.build(); + } } diff --git a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/util/KeyUtilsTest.java b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/util/KeyUtilsTest.java index eb19b12c85..9379349eb4 100644 --- a/data-loader/core/src/test/java/com/scalar/db/dataloader/core/util/KeyUtilsTest.java +++ b/data-loader/core/src/test/java/com/scalar/db/dataloader/core/util/KeyUtilsTest.java @@ -11,6 +11,7 @@ import com.scalar.db.dataloader.core.ColumnInfo; import com.scalar.db.dataloader.core.ColumnKeyValue; import com.scalar.db.dataloader.core.UnitTestUtils; +import com.scalar.db.dataloader.core.exception.ColumnParsingException; import com.scalar.db.dataloader.core.exception.KeyParsingException; import com.scalar.db.io.BigIntColumn; import com.scalar.db.io.BlobColumn; @@ -22,9 +23,11 @@ import com.scalar.db.io.Key; import com.scalar.db.io.TextColumn; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Base64; import java.util.Collections; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -229,4 +232,20 @@ void createPartitionKeyFromSource_withValidData_shouldReturnValidKey() { "Optional[Key{BigIntColumn{name=col1, value=9007199254740992, hasNullValue=false}}]", key.toString()); } + + @Test + void parseMultipleKeyValues_withValidColumns_ShouldReturnValidKey() + throws ColumnParsingException { + String c1 = UnitTestUtils.TEST_COLUMN_2_CK; + ColumnKeyValue k1 = new ColumnKeyValue(c1, "1"); + String c2 = UnitTestUtils.TEST_COLUMN_3_CK; + ColumnKeyValue k2 = new ColumnKeyValue(c2, "false"); + List columnKeyValueList = new ArrayList<>(); + columnKeyValueList.add(k1); + columnKeyValueList.add(k2); + Key key = + KeyUtils.parseMultipleKeyValues( + columnKeyValueList, UnitTestUtils.createTestTableMetadata()); + assertEquals(c1, key.getColumnName(0)); + } } From 7f7d34bf8fd650a5e5ff0444a210235ea009edaa Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Thu, 15 May 2025 09:54:24 +0530 Subject: [PATCH 16/30] Removed unused file --- .../com/scalar/db/dataloader/cli/util/ScalarDbUtils.java | 6 ------ 1 file changed, 6 deletions(-) delete mode 100644 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/ScalarDbUtils.java diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/ScalarDbUtils.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/ScalarDbUtils.java deleted file mode 100644 index 34314f6dc1..0000000000 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/ScalarDbUtils.java +++ /dev/null @@ -1,6 +0,0 @@ -package com.scalar.db.dataloader.cli.util; - -/** The utility class for ScalarDB */ -public class ScalarDbUtils { - public static final String SCALAR_DB_TRANSACTION_MANAGER_CLUSTER = "cluster"; -} From a842dc655e32a4173cf66a891a8eac872ea0bcc4 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Thu, 15 May 2025 10:04:26 +0530 Subject: [PATCH 17/30] Minor name changes --- .../cli/command/dataexport/ExportCommand.java | 10 +++++----- .../db/dataloader/cli/util/DirectoryUtilsTest.java | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java index dd44f043b2..aee68c1313 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java @@ -44,7 +44,7 @@ public class ExportCommand extends ExportCommandOptions implements Callable { private static final String EXPORT_FILE_NAME_FORMAT = "export_%s.%s_%s.%s"; - private static final Logger LOGGER = LoggerFactory.getLogger(ExportCommand.class); + private static final Logger logger = LoggerFactory.getLogger(ExportCommand.class); @Spec CommandSpec spec; @@ -81,7 +81,7 @@ public Integer call() throws Exception { String filePath = getOutputAbsoluteFilePath( outputDirectory, outputFileName, exportOptions.getOutputFileFormat()); - LOGGER.info("Exporting data to file: {}", filePath); + logger.info("Exporting data to file: {}", filePath); try (BufferedWriter writer = Files.newBufferedWriter(Paths.get(filePath), Charset.defaultCharset(), CREATE, APPEND)) { @@ -89,15 +89,15 @@ public Integer call() throws Exception { } } catch (DirectoryValidationException e) { - LOGGER.error("Invalid output directory path: {}", outputDirectory); + logger.error("Invalid output directory path: {}", outputDirectory); return 1; } catch (InvalidFilePathException e) { - LOGGER.error( + logger.error( "The ScalarDB connection settings file path is invalid or the file is missing: {}", scalarDbPropertiesFilePath); return 1; } catch (TableMetadataException e) { - LOGGER.error("Failed to retrieve table metadata: {}", e.getMessage()); + logger.error("Failed to retrieve table metadata: {}", e.getMessage()); return 1; } return 0; diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java index ad4db52f0d..1ff380c19c 100755 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java @@ -18,7 +18,7 @@ /** This class tests the DirectoryValidationUtil class. */ class DirectoryUtilsTest { - private static final Logger LOGGER = LoggerFactory.getLogger(DirectoryUtilsTest.class); + private static final Logger logger = LoggerFactory.getLogger(DirectoryUtilsTest.class); @TempDir Path tempDir; @@ -81,7 +81,7 @@ private void deleteFile(Path file) { try { Files.deleteIfExists(file); } catch (IOException e) { - LOGGER.error("Failed to delete file: {}", file, e); + logger.error("Failed to delete file: {}", file, e); } } } From 508c311eaa9f51a5c22588735e1dd3229ae76e39 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Thu, 15 May 2025 10:28:03 +0530 Subject: [PATCH 18/30] Fixed unit test failure --- .../dataloader/cli/command/dataexport/ExportCommandTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java index 07a78b93b9..28f948532c 100755 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java @@ -2,6 +2,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; +import com.scalar.db.common.error.CoreError; import com.scalar.db.dataloader.core.FileFormat; import java.io.File; import java.nio.file.Paths; @@ -36,7 +37,8 @@ void call_withBlankScalarDBConfigurationFile_shouldThrowException() { IllegalArgumentException.class, exportCommand::call, "Expected to throw FileNotFound exception as configuration path is invalid"); - Assertions.assertEquals("File path must not be blank.", thrown.getMessage()); + Assertions.assertEquals( + CoreError.DATA_LOADER_FILE_PATH_IS_BLANK.buildMessage(), thrown.getMessage()); } @Test From b9b6efde01f3d2be17cef2576cee48f76d4f85e7 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Thu, 15 May 2025 11:50:04 +0530 Subject: [PATCH 19/30] Minor unit test changes --- .../dataexport/MultiColumnKeyValueConverterTest.java | 4 ++-- .../cli/command/dataexport/ScanOrderingConverterTest.java | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java index b7ee8ae36e..71e29b644b 100755 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java @@ -12,7 +12,7 @@ public class MultiColumnKeyValueConverterTest { MultiColumnKeyValueConverter multiColumnKeyValueConverter = new MultiColumnKeyValueConverter(); @Test - public void convert_withInvalidValue_ShouldThrowError() { + void convert_withInvalidValue_ShouldThrowError() { String value = "id 15"; IllegalArgumentException thrown = assertThrows( @@ -23,7 +23,7 @@ public void convert_withInvalidValue_ShouldThrowError() { } @Test - public void convert_withValidValue_ShouldReturnColumnKeyValue() { + void convert_withValidValue_ShouldReturnColumnKeyValue() { String value = "id=15"; ColumnKeyValue expectedOrder = new ColumnKeyValue("id", "15"); Assertions.assertEquals( diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java index 206798514b..897dc10266 100755 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java @@ -13,7 +13,7 @@ public class ScanOrderingConverterTest { ScanOrderingConverter scanOrderingConverter = new ScanOrderingConverter(); @Test - public void callConvert_withInvalidValue_shouldThrowException() { + void callConvert_withInvalidValue_shouldThrowException() { String value = "id ASC"; IllegalArgumentException thrown = assertThrows( @@ -24,7 +24,7 @@ public void callConvert_withInvalidValue_shouldThrowException() { } @Test - public void callConvert_withValidValueAndOrderAscending_shouldReturnScanOrdering() { + void callConvert_withValidValueAndOrderAscending_shouldReturnScanOrdering() { String value = "id=ASC,age=DESC"; List expectedOrder = new ArrayList<>(); expectedOrder.add(new Scan.Ordering("id", Scan.Ordering.Order.ASC)); @@ -33,7 +33,7 @@ public void callConvert_withValidValueAndOrderAscending_shouldReturnScanOrdering } @Test - public void callConvert_withValidValueAndOrderDescending_shouldReturnScanOrdering() { + void callConvert_withValidValueAndOrderDescending_shouldReturnScanOrdering() { String value = "id=desc"; List expectedOrder = Collections.singletonList(new Scan.Ordering("id", Scan.Ordering.Order.DESC)); From c83906685b64d743fef6b699bb39a6648a36e828 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Thu, 15 May 2025 16:35:47 +0530 Subject: [PATCH 20/30] Desription and default value corrections --- .../cli/command/dataexport/ExportCommandOptions.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java index aa88c87343..4ae0e2b1ed 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java @@ -56,7 +56,7 @@ public class ExportCommandOptions { @CommandLine.Option( names = {"--format", "-fmt"}, paramLabel = "", - description = "Format of the exported data file (json, csv, tsv) (default: json)", + description = "Format of the exported data file (json, csv, jsonl) (default: json)", defaultValue = "json") protected FileFormat outputFormat; @@ -123,13 +123,13 @@ public class ExportCommandOptions { @CommandLine.Option( names = {"--delimiter"}, paramLabel = "", - defaultValue = ";", - description = "Delimiter character for CSV/TSV files (default: comma for CSV, tab for TSV)") + defaultValue = ",", + description = "Delimiter character for CSV files (default: comma)") protected String delimiter; @CommandLine.Option( names = {"--no-header", "-nh"}, - description = "Exclude header row in CSV/TSV files (default: false)", + description = "Exclude header row in CSV files (default: false)", defaultValue = "false") protected boolean excludeHeader; From 6b0594850668fcfe1a8c5d47e2319966eefecf60 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Mon, 19 May 2025 17:11:09 +0530 Subject: [PATCH 21/30] Changes --- .../com/scalar/db/common/error/CoreError.java | 12 ++ .../cli/command/dataexport/ExportCommand.java | 2 +- .../dataexport/ExportCommandOptions.java | 2 +- .../MultiColumnKeyValueConverter.java | 26 ++--- .../dataexport/ScanOrderingConverter.java | 11 +- .../SingleColumnKeyValueConverter.java | 22 +--- .../cli/util/CommandLineInputUtils.java | 48 ++++++++ .../MultiColumnKeyValueConverterTest.java | 4 +- .../dataexport/ScanOrderingConverterTest.java | 4 +- .../cli/util/CommandLineInputUtilsTest.java | 105 ++++++++++++++++++ 10 files changed, 189 insertions(+), 47 deletions(-) create mode 100644 data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java create mode 100644 data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java diff --git a/core/src/main/java/com/scalar/db/common/error/CoreError.java b/core/src/main/java/com/scalar/db/common/error/CoreError.java index 6185493f20..60e732b16a 100644 --- a/core/src/main/java/com/scalar/db/common/error/CoreError.java +++ b/core/src/main/java/com/scalar/db/common/error/CoreError.java @@ -896,6 +896,18 @@ public enum CoreError implements ScalarDbError { DATA_LOADER_FILE_PATH_IS_BLANK( Category.USER_ERROR, "0197", "File path must not be blank.", "", ""), DATA_LOADER_FILE_NOT_FOUND(Category.USER_ERROR, "0198", "File not found: %s", "", ""), + DATA_LOADER_NULL_OR_EMPTY_KEY_VALUE_INPUT( + Category.USER_ERROR, "0200", "Key-value cannot be null or empty", "", ""), + DATA_LOADER_INVALID_KEY_VALUE_INPUT( + Category.USER_ERROR, "0201", "Invalid key-value format: %s", "", ""), + DATA_LOADER_INVALID_COLUMN_ORDER_FORMAT( + Category.USER_ERROR, "0202", "Invalid column order format: %s", "", ""), + + DATA_LOADER_SPLIT_INPUT_VALUE_NULL( + Category.USER_ERROR, "0202", "Invalid column order format: %s", "", ""), + + DATA_LOADER_SPLIT_INPUT_DELIMITER_NULL( + Category.USER_ERROR, "0202", "Invalid column order format: %s", "", ""), // // Errors for the concurrency error category diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java index aee68c1313..4875e69a30 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java @@ -43,7 +43,7 @@ @CommandLine.Command(name = "export", description = "export data from a ScalarDB table") public class ExportCommand extends ExportCommandOptions implements Callable { - private static final String EXPORT_FILE_NAME_FORMAT = "export_%s.%s_%s.%s"; + private static final String EXPORT_FILE_NAME_FORMAT = "export.%s.%s.%s.%s"; private static final Logger logger = LoggerFactory.getLogger(ExportCommand.class); @Spec CommandSpec spec; diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java index 4ae0e2b1ed..5cbe8f6c78 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java @@ -36,7 +36,7 @@ public class ExportCommandOptions { names = {"--output-file", "-o"}, paramLabel = "", description = - "Name of the output file for the exported data (default: .)") + "Name of the output file for the exported data (default: export..
    ..)") protected String outputFileName; @CommandLine.Option( diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java index 955ae66966..c96a09384e 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java @@ -1,5 +1,9 @@ package com.scalar.db.dataloader.cli.command.dataexport; +import static com.scalar.db.dataloader.cli.util.CommandLineInputUtils.parseKeyValue; +import static com.scalar.db.dataloader.cli.util.CommandLineInputUtils.splitByDelimiter; + +import com.scalar.db.common.error.CoreError; import com.scalar.db.dataloader.core.ColumnKeyValue; import java.util.ArrayList; import java.util.List; @@ -33,11 +37,12 @@ public class MultiColumnKeyValueConverter @Override public List convert(String keyValue) { if (keyValue == null || keyValue.trim().isEmpty()) { - throw new IllegalArgumentException("Key-value cannot be null or empty"); + throw new IllegalArgumentException( + CoreError.DATA_LOADER_NULL_OR_EMPTY_KEY_VALUE_INPUT.buildMessage()); } List columnKeyValueList = new ArrayList<>(); - String[] columnValues = keyValue.split(","); + String[] columnValues = splitByDelimiter(keyValue, ",", 0); for (String columnValue : columnValues) { columnKeyValueList.add(parseKeyValue(columnValue)); @@ -45,21 +50,4 @@ public List convert(String keyValue) { return columnKeyValueList; } - - /** - * Parses a single key-value pair from a string in the format "key=value". - * - * @param keyValue the key-value string to parse - * @return a {@link ColumnKeyValue} object representing the parsed key-value pair - * @throws IllegalArgumentException if the input is not in the expected format - */ - private ColumnKeyValue parseKeyValue(String keyValue) { - String[] parts = keyValue.split("=", 2); - - if (parts.length != 2 || parts[0].trim().isEmpty() || parts[1].trim().isEmpty()) { - throw new IllegalArgumentException("Invalid key-value format: " + keyValue); - } - - return new ColumnKeyValue(parts[0].trim(), parts[1].trim()); - } } diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java index fc70bfd07a..2ba9c99992 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java @@ -1,6 +1,9 @@ package com.scalar.db.dataloader.cli.command.dataexport; +import static com.scalar.db.dataloader.cli.util.CommandLineInputUtils.splitByDelimiter; + import com.scalar.db.api.Scan; +import com.scalar.db.common.error.CoreError; import java.util.ArrayList; import java.util.List; import picocli.CommandLine; @@ -8,13 +11,13 @@ public class ScanOrderingConverter implements CommandLine.ITypeConverter> { @Override public List convert(String value) { - String[] parts = value.split(","); + String[] parts = splitByDelimiter(value, ",", 0); List scanOrderList = new ArrayList<>(); for (String part : parts) { - String[] data = part.split("="); - + String[] data = splitByDelimiter(part, "=", 2); if (data.length != 2) { - throw new IllegalArgumentException("Invalid column order format: " + value); + throw new IllegalArgumentException( + CoreError.DATA_LOADER_INVALID_COLUMN_ORDER_FORMAT.buildMessage(value)); } String columnName = data[0].trim(); Scan.Ordering.Order sortOrder = Scan.Ordering.Order.valueOf(data[1].trim().toUpperCase()); diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java index 4a61958e74..3028e07403 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java @@ -1,5 +1,7 @@ package com.scalar.db.dataloader.cli.command.dataexport; +import static com.scalar.db.dataloader.cli.util.CommandLineInputUtils.parseKeyValue; + import com.scalar.db.dataloader.core.ColumnKeyValue; import picocli.CommandLine; @@ -18,26 +20,6 @@ public class SingleColumnKeyValueConverter implements CommandLine.ITypeConverter */ @Override public ColumnKeyValue convert(String keyValue) { - if (keyValue == null || keyValue.trim().isEmpty()) { - throw new IllegalArgumentException("Key-value cannot be null or empty"); - } return parseKeyValue(keyValue); } - - /** - * Parses a single key-value pair from a string in the format "key=value". - * - * @param keyValue the key-value string to parse - * @return a {@link ColumnKeyValue} object representing the parsed key-value pair - * @throws IllegalArgumentException if the input is not in the expected format - */ - private ColumnKeyValue parseKeyValue(String keyValue) { - String[] parts = keyValue.split("=", 2); - - if (parts.length != 2 || parts[0].trim().isEmpty() || parts[1].trim().isEmpty()) { - throw new IllegalArgumentException("Invalid key-value format: " + keyValue); - } - - return new ColumnKeyValue(parts[0].trim(), parts[1].trim()); - } } diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java new file mode 100644 index 0000000000..0e88936213 --- /dev/null +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java @@ -0,0 +1,48 @@ +package com.scalar.db.dataloader.cli.util; + +import com.scalar.db.common.error.CoreError; +import com.scalar.db.dataloader.core.ColumnKeyValue; +import java.util.Objects; + +public class CommandLineInputUtils { + + /** + * Parses a single key-value pair from a string in the format "key=value". + * + * @param keyValue the key-value string to parse + * @return a {@link ColumnKeyValue} object representing the parsed key-value pair + * @throws IllegalArgumentException if the input is null, empty, or not in the expected format + */ + public static ColumnKeyValue parseKeyValue(String keyValue) { + if (keyValue == null || keyValue.trim().isEmpty()) { + throw new IllegalArgumentException( + CoreError.DATA_LOADER_NULL_OR_EMPTY_KEY_VALUE_INPUT.buildMessage()); + } + + String[] parts = splitByDelimiter(keyValue, "=", 2); + + if (parts.length != 2 || parts[0].trim().isEmpty() || parts[1].trim().isEmpty()) { + throw new IllegalArgumentException( + CoreError.DATA_LOADER_INVALID_KEY_VALUE_INPUT.buildMessage(keyValue)); + } + + return new ColumnKeyValue(parts[0].trim(), parts[1].trim()); + } + + /** + * Splits a string based on the provided delimiter. + * + * @param value the string to split + * @param delimiter the delimiter to use + * @param limit the maximum number of elements in the result (same behavior as String.split() with + * limit) + * @return an array of split values + * @throws NullPointerException if value or delimiter is null + */ + public static String[] splitByDelimiter(String value, String delimiter, int limit) { + Objects.requireNonNull(value, CoreError.DATA_LOADER_SPLIT_INPUT_VALUE_NULL.buildMessage()); + Objects.requireNonNull( + delimiter, CoreError.DATA_LOADER_SPLIT_INPUT_DELIMITER_NULL.buildMessage()); + return value.split(delimiter, limit); + } +} diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java index 71e29b644b..89adb8f077 100755 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverterTest.java @@ -2,6 +2,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; +import com.scalar.db.common.error.CoreError; import com.scalar.db.dataloader.core.ColumnKeyValue; import java.util.Collections; import org.junit.jupiter.api.Assertions; @@ -19,7 +20,8 @@ void convert_withInvalidValue_ShouldThrowError() { IllegalArgumentException.class, () -> multiColumnKeyValueConverter.convert(value), "Expected to throw exception"); - Assertions.assertEquals("Invalid key-value format: id 15", thrown.getMessage()); + Assertions.assertEquals( + CoreError.DATA_LOADER_INVALID_KEY_VALUE_INPUT.buildMessage("id 15"), thrown.getMessage()); } @Test diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java index 897dc10266..fe0caacb9b 100755 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java @@ -3,6 +3,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import com.scalar.db.api.Scan; +import com.scalar.db.common.error.CoreError; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -20,7 +21,8 @@ void callConvert_withInvalidValue_shouldThrowException() { IllegalArgumentException.class, () -> scanOrderingConverter.convert(value), "Expected to throw exception"); - Assertions.assertEquals("Invalid column order format: id ASC", thrown.getMessage()); + Assertions.assertEquals( + CoreError.DATA_LOADER_INVALID_COLUMN_ORDER_FORMAT.buildMessage(value), thrown.getMessage()); } @Test diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java new file mode 100644 index 0000000000..3c86ce736a --- /dev/null +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java @@ -0,0 +1,105 @@ +package com.scalar.db.dataloader.cli.util; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.scalar.db.common.error.CoreError; +import com.scalar.db.dataloader.core.ColumnKeyValue; +import org.junit.jupiter.api.Test; + +class CommandLineInputUtilsTest { + + @Test + void parseKeyValue_validInput_shouldReturnKeyValue() { + ColumnKeyValue result = CommandLineInputUtils.parseKeyValue("name=John"); + assertEquals("name", result.getColumnName()); + assertEquals("John", result.getColumnValue()); + } + + @Test + void parseKeyValue_noEqualSign_shouldThrowException() { + IllegalArgumentException exception = + assertThrows( + IllegalArgumentException.class, + () -> CommandLineInputUtils.parseKeyValue("invalidpair")); + assertTrue(exception.getMessage().contains("Invalid key-value format")); + } + + @Test + void parseKeyValue_emptyKey_shouldThrowException() { + IllegalArgumentException exception = + assertThrows( + IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue("=value")); + assertTrue(exception.getMessage().contains("Invalid key-value format")); + } + + @Test + void parseKeyValue_emptyValue_shouldThrowException() { + IllegalArgumentException exception = + assertThrows( + IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue("key=")); + assertTrue( + exception + .getMessage() + .contains(CoreError.DATA_LOADER_INVALID_KEY_VALUE_INPUT.buildMessage("key="))); + } + + @Test + void parseKeyValue_blankInput_shouldThrowException() { + IllegalArgumentException exception = + assertThrows( + IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue(" ")); + assertTrue( + exception + .getMessage() + .contains(CoreError.DATA_LOADER_NULL_OR_EMPTY_KEY_VALUE_INPUT.buildMessage())); + } + + @Test + void parseKeyValue_nullInput_shouldThrowException() { + IllegalArgumentException exception = + assertThrows( + IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue(null)); + assertTrue( + exception + .getMessage() + .contains(CoreError.DATA_LOADER_NULL_OR_EMPTY_KEY_VALUE_INPUT.buildMessage())); + } + + @Test + void splitByDelimiter_validSplit_shouldReturnArray() { + String[] result = CommandLineInputUtils.splitByDelimiter("a=b", "=", 2); + assertArrayEquals(new String[] {"a", "b"}, result); + } + + @Test + void splitByDelimiter_multipleDelimiters_shouldSplitAll() { + String[] result = CommandLineInputUtils.splitByDelimiter("a=b=c", "=", 0); + assertArrayEquals(new String[] {"a", "b", "c"}, result); + } + + @Test + void splitByDelimiter_nullValue_shouldThrowException() { + NullPointerException exception = + assertThrows( + NullPointerException.class, () -> CommandLineInputUtils.splitByDelimiter(null, "=", 2)); + assertTrue( + exception + .getMessage() + .contains(CoreError.DATA_LOADER_SPLIT_INPUT_VALUE_NULL.buildMessage())); + } + + @Test + void splitByDelimiter_nullDelimiter_shouldThrowException() { + NullPointerException exception = + assertThrows( + NullPointerException.class, + () -> CommandLineInputUtils.splitByDelimiter("a=b", null, 2)); + assertTrue( + exception + .getMessage() + .contains(CoreError.DATA_LOADER_SPLIT_INPUT_DELIMITER_NULL.buildMessage())); + } +} From 707f56e1245cb9d393ff564f7af8b59737fdd83b Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Mon, 19 May 2025 17:25:43 +0530 Subject: [PATCH 22/30] Fix error message --- core/src/main/java/com/scalar/db/common/error/CoreError.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/scalar/db/common/error/CoreError.java b/core/src/main/java/com/scalar/db/common/error/CoreError.java index 60e732b16a..1fed10da79 100644 --- a/core/src/main/java/com/scalar/db/common/error/CoreError.java +++ b/core/src/main/java/com/scalar/db/common/error/CoreError.java @@ -903,11 +903,10 @@ public enum CoreError implements ScalarDbError { DATA_LOADER_INVALID_COLUMN_ORDER_FORMAT( Category.USER_ERROR, "0202", "Invalid column order format: %s", "", ""), - DATA_LOADER_SPLIT_INPUT_VALUE_NULL( - Category.USER_ERROR, "0202", "Invalid column order format: %s", "", ""), + DATA_LOADER_SPLIT_INPUT_VALUE_NULL(Category.USER_ERROR, "0203", "Value must not be null", "", ""), DATA_LOADER_SPLIT_INPUT_DELIMITER_NULL( - Category.USER_ERROR, "0202", "Invalid column order format: %s", "", ""), + Category.USER_ERROR, "0204", "Delimiter must not be null", "", ""), // // Errors for the concurrency error category From 145b2391345a9159e76eb4beded9e3496d0490a8 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Tue, 20 May 2025 14:53:02 +0530 Subject: [PATCH 23/30] Javadoc change --- .../java/com/scalar/db/dataloader/core/util/KeyUtils.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/KeyUtils.java b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/KeyUtils.java index 45f1ad22e2..f20e013052 100644 --- a/data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/KeyUtils.java +++ b/data-loader/core/src/main/java/com/scalar/db/dataloader/core/util/KeyUtils.java @@ -215,9 +215,10 @@ private static Optional createKeyFromSource( } /** - * Convert a keyValue, in the format of =, to a ScalarDB Key instance. + * Convert a list of ColumnKeyValue objects to a ScalarDB Key instance. * - * @param keyValues A list of key values in the format of = + * @param keyValues A list of ColumnKeyValue objects, where each object contains a column name and + * its corresponding value * @param tableMetadata Metadata for one ScalarDB table * @return A new ScalarDB Key instance formatted by data type * @throws ColumnParsingException if there is an error parsing the column From 210f62d5d853a9f81939bef3df702b6c607c312d Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Tue, 20 May 2025 17:01:07 +0530 Subject: [PATCH 24/30] Changed exception thrown based on feedback --- .../main/java/com/scalar/db/dataloader/cli/util/FileUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java index ecaf341409..37c8ce3b57 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java @@ -15,7 +15,7 @@ public class FileUtils { */ public static void validateFilePath(String filePath) throws InvalidFilePathException { if (StringUtils.isBlank(filePath)) { - throw new IllegalArgumentException(CoreError.DATA_LOADER_FILE_PATH_IS_BLANK.buildMessage()); + throw new InvalidFilePathException(CoreError.DATA_LOADER_FILE_PATH_IS_BLANK.buildMessage()); } Path pathToCheck = Paths.get(filePath); From 676c12d3874f46456ea127c180101626bf9e67d0 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Tue, 20 May 2025 17:04:24 +0530 Subject: [PATCH 25/30] Revert "Changed exception thrown based on feedback" This reverts commit 210f62d5d853a9f81939bef3df702b6c607c312d. --- .../main/java/com/scalar/db/dataloader/cli/util/FileUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java index 37c8ce3b57..ecaf341409 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/FileUtils.java @@ -15,7 +15,7 @@ public class FileUtils { */ public static void validateFilePath(String filePath) throws InvalidFilePathException { if (StringUtils.isBlank(filePath)) { - throw new InvalidFilePathException(CoreError.DATA_LOADER_FILE_PATH_IS_BLANK.buildMessage()); + throw new IllegalArgumentException(CoreError.DATA_LOADER_FILE_PATH_IS_BLANK.buildMessage()); } Path pathToCheck = Paths.get(filePath); From d05ec669c171eca564c435b123f2c073b11e6428 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Wed, 21 May 2025 10:27:33 +0530 Subject: [PATCH 26/30] Changes --- .../MultiColumnKeyValueConverter.java | 20 ++--- .../SingleColumnKeyValueConverter.java | 4 +- .../cli/util/CommandLineInputUtils.java | 10 +-- .../cli/util/CommandLineInputUtilsTest.java | 75 +++++++++---------- 4 files changed, 50 insertions(+), 59 deletions(-) diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java index c96a09384e..6479089f18 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/MultiColumnKeyValueConverter.java @@ -1,12 +1,11 @@ package com.scalar.db.dataloader.cli.command.dataexport; -import static com.scalar.db.dataloader.cli.util.CommandLineInputUtils.parseKeyValue; -import static com.scalar.db.dataloader.cli.util.CommandLineInputUtils.splitByDelimiter; - import com.scalar.db.common.error.CoreError; +import com.scalar.db.dataloader.cli.util.CommandLineInputUtils; import com.scalar.db.dataloader.core.ColumnKeyValue; -import java.util.ArrayList; +import java.util.Arrays; import java.util.List; +import java.util.stream.Collectors; import picocli.CommandLine; /** @@ -40,14 +39,9 @@ public List convert(String keyValue) { throw new IllegalArgumentException( CoreError.DATA_LOADER_NULL_OR_EMPTY_KEY_VALUE_INPUT.buildMessage()); } - - List columnKeyValueList = new ArrayList<>(); - String[] columnValues = splitByDelimiter(keyValue, ",", 0); - - for (String columnValue : columnValues) { - columnKeyValueList.add(parseKeyValue(columnValue)); - } - - return columnKeyValueList; + return Arrays.stream(CommandLineInputUtils.splitByDelimiter(keyValue, ",", 0)) + .map(CommandLineInputUtils::parseKeyValue) + .map(entry -> new ColumnKeyValue(entry.getKey(), entry.getValue())) + .collect(Collectors.toList()); } } diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java index 3028e07403..b96e48247d 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/SingleColumnKeyValueConverter.java @@ -3,6 +3,7 @@ import static com.scalar.db.dataloader.cli.util.CommandLineInputUtils.parseKeyValue; import com.scalar.db.dataloader.core.ColumnKeyValue; +import java.util.Map; import picocli.CommandLine; /** @@ -20,6 +21,7 @@ public class SingleColumnKeyValueConverter implements CommandLine.ITypeConverter */ @Override public ColumnKeyValue convert(String keyValue) { - return parseKeyValue(keyValue); + Map.Entry data = parseKeyValue(keyValue); + return new ColumnKeyValue(data.getKey(), data.getValue()); } } diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java index 0e88936213..1f476a5ea1 100644 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java @@ -1,7 +1,8 @@ package com.scalar.db.dataloader.cli.util; import com.scalar.db.common.error.CoreError; -import com.scalar.db.dataloader.core.ColumnKeyValue; +import java.util.AbstractMap; +import java.util.Map; import java.util.Objects; public class CommandLineInputUtils { @@ -10,10 +11,10 @@ public class CommandLineInputUtils { * Parses a single key-value pair from a string in the format "key=value". * * @param keyValue the key-value string to parse - * @return a {@link ColumnKeyValue} object representing the parsed key-value pair + * @return a {@link Map.Entry} representing the parsed key-value pair * @throws IllegalArgumentException if the input is null, empty, or not in the expected format */ - public static ColumnKeyValue parseKeyValue(String keyValue) { + public static Map.Entry parseKeyValue(String keyValue) { if (keyValue == null || keyValue.trim().isEmpty()) { throw new IllegalArgumentException( CoreError.DATA_LOADER_NULL_OR_EMPTY_KEY_VALUE_INPUT.buildMessage()); @@ -25,8 +26,7 @@ public static ColumnKeyValue parseKeyValue(String keyValue) { throw new IllegalArgumentException( CoreError.DATA_LOADER_INVALID_KEY_VALUE_INPUT.buildMessage(keyValue)); } - - return new ColumnKeyValue(parts[0].trim(), parts[1].trim()); + return new AbstractMap.SimpleEntry<>(parts[0].trim(), parts[1].trim()); } /** diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java index 3c86ce736a..7faebef61a 100644 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java @@ -6,66 +6,61 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import com.scalar.db.common.error.CoreError; -import com.scalar.db.dataloader.core.ColumnKeyValue; +import java.util.Map; import org.junit.jupiter.api.Test; class CommandLineInputUtilsTest { @Test - void parseKeyValue_validInput_shouldReturnKeyValue() { - ColumnKeyValue result = CommandLineInputUtils.parseKeyValue("name=John"); - assertEquals("name", result.getColumnName()); - assertEquals("John", result.getColumnValue()); + public void parseKeyValue_validKeyValue_ShouldReturnEntry() { + Map.Entry result = CommandLineInputUtils.parseKeyValue("foo=bar"); + + assertEquals("foo", result.getKey()); + assertEquals("bar", result.getValue()); } @Test - void parseKeyValue_noEqualSign_shouldThrowException() { - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, - () -> CommandLineInputUtils.parseKeyValue("invalidpair")); - assertTrue(exception.getMessage().contains("Invalid key-value format")); + public void parseKeyValue_whitespaceTrimmed_ShouldReturnTrimmedEntry() { + Map.Entry result = CommandLineInputUtils.parseKeyValue(" key = value "); + + assertEquals("key", result.getKey()); + assertEquals("value", result.getValue()); } @Test - void parseKeyValue_emptyKey_shouldThrowException() { - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue("=value")); - assertTrue(exception.getMessage().contains("Invalid key-value format")); + public void parseKeyValue_nullInput_ShouldThrowException() { + assertThrows(IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue(null)); } @Test - void parseKeyValue_emptyValue_shouldThrowException() { - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue("key=")); - assertTrue( - exception - .getMessage() - .contains(CoreError.DATA_LOADER_INVALID_KEY_VALUE_INPUT.buildMessage("key="))); + public void parseKeyValue_emptyInput_ShouldThrowException() { + assertThrows(IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue(" ")); } @Test - void parseKeyValue_blankInput_shouldThrowException() { - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue(" ")); - assertTrue( - exception - .getMessage() - .contains(CoreError.DATA_LOADER_NULL_OR_EMPTY_KEY_VALUE_INPUT.buildMessage())); + public void parseKeyValue_missingEquals_ShouldThrowException() { + assertThrows( + IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue("keyvalue")); } @Test - void parseKeyValue_nullInput_shouldThrowException() { - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue(null)); - assertTrue( - exception - .getMessage() - .contains(CoreError.DATA_LOADER_NULL_OR_EMPTY_KEY_VALUE_INPUT.buildMessage())); + public void parseKeyValue_emptyKey_ShouldThrowException() { + assertThrows( + IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue(" =value")); + } + + @Test + public void parseKeyValue_emptyValue_ShouldThrowException() { + assertThrows( + IllegalArgumentException.class, () -> CommandLineInputUtils.parseKeyValue("key= ")); + } + + @Test + public void parseKeyValue_multipleEquals_ShouldParseFirstOnly() { + Map.Entry result = CommandLineInputUtils.parseKeyValue("key=val=ue"); + + assertEquals("key", result.getKey()); + assertEquals("val=ue", result.getValue()); } @Test From e0ecb75015e65d05cfc76a1147e9b8b19bb82f92 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Wed, 21 May 2025 11:47:34 +0530 Subject: [PATCH 27/30] Additional check added --- core/src/main/java/com/scalar/db/common/error/CoreError.java | 2 ++ .../db/dataloader/cli/command/dataexport/ExportCommand.java | 5 +++++ .../dataloader/cli/command/dataexport/ExportCommandTest.java | 2 +- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/scalar/db/common/error/CoreError.java b/core/src/main/java/com/scalar/db/common/error/CoreError.java index d5aa0e09af..4bb2d26bbb 100644 --- a/core/src/main/java/com/scalar/db/common/error/CoreError.java +++ b/core/src/main/java/com/scalar/db/common/error/CoreError.java @@ -913,6 +913,8 @@ public enum CoreError implements ScalarDbError { DATA_LOADER_SPLIT_INPUT_DELIMITER_NULL( Category.USER_ERROR, "0204", "Delimiter must not be null", "", ""), + DATA_LOADER_CONFIG_FILE_PATH_BLANK( + Category.USER_ERROR, "0205", "Config file path must not be blank", "", ""), // // Errors for the concurrency error category diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java index 4875e69a30..96bcc87c72 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java @@ -5,6 +5,7 @@ import com.scalar.db.api.DistributedStorage; import com.scalar.db.api.TableMetadata; +import com.scalar.db.common.error.CoreError; import com.scalar.db.dataloader.cli.exception.DirectoryValidationException; import com.scalar.db.dataloader.cli.util.DirectoryUtils; import com.scalar.db.dataloader.cli.util.FileUtils; @@ -104,6 +105,10 @@ public Integer call() throws Exception { } private String getScalarDbPropertiesFilePath() { + if (configFilePath == null || configFilePath.trim().isEmpty()) { + throw new IllegalArgumentException( + CoreError.DATA_LOADER_CONFIG_FILE_PATH_BLANK.buildMessage()); + } return Objects.equals(configFilePath, DEFAULT_CONFIG_FILE_NAME) ? Paths.get("").toAbsolutePath().resolve(DEFAULT_CONFIG_FILE_NAME).toString() : configFilePath; diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java index 28f948532c..73934f340d 100755 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java @@ -38,7 +38,7 @@ void call_withBlankScalarDBConfigurationFile_shouldThrowException() { exportCommand::call, "Expected to throw FileNotFound exception as configuration path is invalid"); Assertions.assertEquals( - CoreError.DATA_LOADER_FILE_PATH_IS_BLANK.buildMessage(), thrown.getMessage()); + CoreError.DATA_LOADER_CONFIG_FILE_PATH_BLANK.buildMessage(), thrown.getMessage()); } @Test From f595434273b5b7e93bcb929a212d6e7ada754a2a Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Wed, 21 May 2025 11:52:27 +0530 Subject: [PATCH 28/30] Minor changes --- .../db/dataloader/cli/command/dataexport/ExportCommand.java | 2 +- .../scalar/db/dataloader/cli/util/CommandLineInputUtils.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java index 96bcc87c72..fdedbeef2c 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java @@ -105,7 +105,7 @@ public Integer call() throws Exception { } private String getScalarDbPropertiesFilePath() { - if (configFilePath == null || configFilePath.trim().isEmpty()) { + if (StringUtils.isBlank(configFilePath)) { throw new IllegalArgumentException( CoreError.DATA_LOADER_CONFIG_FILE_PATH_BLANK.buildMessage()); } diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java index 1f476a5ea1..88e2188656 100644 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java @@ -4,6 +4,7 @@ import java.util.AbstractMap; import java.util.Map; import java.util.Objects; +import org.apache.commons.lang3.StringUtils; public class CommandLineInputUtils { @@ -15,7 +16,7 @@ public class CommandLineInputUtils { * @throws IllegalArgumentException if the input is null, empty, or not in the expected format */ public static Map.Entry parseKeyValue(String keyValue) { - if (keyValue == null || keyValue.trim().isEmpty()) { + if (StringUtils.isBlank(keyValue)) { throw new IllegalArgumentException( CoreError.DATA_LOADER_NULL_OR_EMPTY_KEY_VALUE_INPUT.buildMessage()); } From 8366167ae977cd6229553b0f69fb40610efa0366 Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Wed, 21 May 2025 13:34:56 +0530 Subject: [PATCH 29/30] Scan order input read updated --- .../com/scalar/db/common/error/CoreError.java | 9 ++--- .../dataexport/ScanOrderingConverter.java | 39 +++++++++++-------- .../dataexport/ScanOrderingConverterTest.java | 2 +- 3 files changed, 26 insertions(+), 24 deletions(-) diff --git a/core/src/main/java/com/scalar/db/common/error/CoreError.java b/core/src/main/java/com/scalar/db/common/error/CoreError.java index 4bb2d26bbb..7a0e5d9efa 100644 --- a/core/src/main/java/com/scalar/db/common/error/CoreError.java +++ b/core/src/main/java/com/scalar/db/common/error/CoreError.java @@ -906,15 +906,12 @@ public enum CoreError implements ScalarDbError { Category.USER_ERROR, "0200", "Key-value cannot be null or empty", "", ""), DATA_LOADER_INVALID_KEY_VALUE_INPUT( Category.USER_ERROR, "0201", "Invalid key-value format: %s", "", ""), - DATA_LOADER_INVALID_COLUMN_ORDER_FORMAT( - Category.USER_ERROR, "0202", "Invalid column order format: %s", "", ""), - - DATA_LOADER_SPLIT_INPUT_VALUE_NULL(Category.USER_ERROR, "0203", "Value must not be null", "", ""), + DATA_LOADER_SPLIT_INPUT_VALUE_NULL(Category.USER_ERROR, "0202", "Value must not be null", "", ""), DATA_LOADER_SPLIT_INPUT_DELIMITER_NULL( - Category.USER_ERROR, "0204", "Delimiter must not be null", "", ""), + Category.USER_ERROR, "0203", "Delimiter must not be null", "", ""), DATA_LOADER_CONFIG_FILE_PATH_BLANK( - Category.USER_ERROR, "0205", "Config file path must not be blank", "", ""), + Category.USER_ERROR, "0204", "Config file path must not be blank", "", ""), // // Errors for the concurrency error category diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java index 2ba9c99992..44267017ca 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverter.java @@ -1,28 +1,33 @@ package com.scalar.db.dataloader.cli.command.dataexport; -import static com.scalar.db.dataloader.cli.util.CommandLineInputUtils.splitByDelimiter; - import com.scalar.db.api.Scan; -import com.scalar.db.common.error.CoreError; -import java.util.ArrayList; +import com.scalar.db.dataloader.cli.util.CommandLineInputUtils; +import java.util.Arrays; import java.util.List; +import java.util.stream.Collectors; import picocli.CommandLine; public class ScanOrderingConverter implements CommandLine.ITypeConverter> { + /** + * Converts a comma-separated string of key-value pairs into a list of {@link Scan.Ordering} + * objects. Each pair must be in the format "column=order", where "order" is a valid enum value of + * {@link Scan.Ordering.Order} (e.g., ASC or DESC, case-insensitive). + * + * @param value the comma-separated key-value string to convert + * @return a list of {@link Scan.Ordering} objects constructed from the input + * @throws IllegalArgumentException if parsing fails due to invalid format or enum value + */ @Override public List convert(String value) { - String[] parts = splitByDelimiter(value, ",", 0); - List scanOrderList = new ArrayList<>(); - for (String part : parts) { - String[] data = splitByDelimiter(part, "=", 2); - if (data.length != 2) { - throw new IllegalArgumentException( - CoreError.DATA_LOADER_INVALID_COLUMN_ORDER_FORMAT.buildMessage(value)); - } - String columnName = data[0].trim(); - Scan.Ordering.Order sortOrder = Scan.Ordering.Order.valueOf(data[1].trim().toUpperCase()); - scanOrderList.add(new Scan.Ordering(columnName, sortOrder)); - } - return scanOrderList; + return Arrays.stream(CommandLineInputUtils.splitByDelimiter(value, ",", 0)) + .map(CommandLineInputUtils::parseKeyValue) + .map( + entry -> { + String columnName = entry.getKey(); + Scan.Ordering.Order sortOrder = + Scan.Ordering.Order.valueOf(entry.getValue().trim().toUpperCase()); + return new Scan.Ordering(columnName, sortOrder); + }) + .collect(Collectors.toList()); } } diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java index fe0caacb9b..e8836ae156 100755 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ScanOrderingConverterTest.java @@ -22,7 +22,7 @@ void callConvert_withInvalidValue_shouldThrowException() { () -> scanOrderingConverter.convert(value), "Expected to throw exception"); Assertions.assertEquals( - CoreError.DATA_LOADER_INVALID_COLUMN_ORDER_FORMAT.buildMessage(value), thrown.getMessage()); + CoreError.DATA_LOADER_INVALID_KEY_VALUE_INPUT.buildMessage(value), thrown.getMessage()); } @Test From 89922013921c9d60249d2c78d0ea25911e12265a Mon Sep 17 00:00:00 2001 From: Jishnu J Date: Thu, 22 May 2025 10:20:01 +0530 Subject: [PATCH 30/30] Empty line removed --- core/src/main/java/com/scalar/db/common/error/CoreError.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/java/com/scalar/db/common/error/CoreError.java b/core/src/main/java/com/scalar/db/common/error/CoreError.java index 7a0e5d9efa..acaa4566dd 100644 --- a/core/src/main/java/com/scalar/db/common/error/CoreError.java +++ b/core/src/main/java/com/scalar/db/common/error/CoreError.java @@ -907,7 +907,6 @@ public enum CoreError implements ScalarDbError { DATA_LOADER_INVALID_KEY_VALUE_INPUT( Category.USER_ERROR, "0201", "Invalid key-value format: %s", "", ""), DATA_LOADER_SPLIT_INPUT_VALUE_NULL(Category.USER_ERROR, "0202", "Value must not be null", "", ""), - DATA_LOADER_SPLIT_INPUT_DELIMITER_NULL( Category.USER_ERROR, "0203", "Delimiter must not be null", "", ""), DATA_LOADER_CONFIG_FILE_PATH_BLANK(