Skip to content

Set --log-raw-record default to true for all failure logs #3185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Set --log-raw-record default to true for all failure logs #3185

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
815e2d4
update default value and comments
inv-jishnu Nov 19, 2025
58c5051
Feedback changes
inv-jishnu Nov 19, 2025
baa94ce
Conditional check added for log raw records
inv-jishnu Nov 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions ...i/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public class ExportCommandOptions {
protected Integer maxThreads;

/**
* @deprecated As of release 3.6.2. Will be removed in release 4.0.0. Use --max-threads instead
* @deprecated As of release 3.17.0. Will be removed in release 4.0.0. Use --max-threads instead
*/
@Deprecated
@CommandLine.Option(
Expand All @@ -118,7 +118,10 @@ public class ExportCommandOptions {
// TODO: test that -si false, works
protected boolean scanStartInclusive;

// Deprecated option - kept for backward compatibility
/**
* @deprecated As of release 3.17.0. Will be removed in release 4.0.0. Use --start-inclusive
* instead (inverted logic).
*/
@CommandLine.Option(
names = {DEPRECATED_START_EXCLUSIVE_OPTION},
description = "Deprecated: Use --start-inclusive instead (inverted logic)",
Expand All @@ -139,7 +142,10 @@ public class ExportCommandOptions {
defaultValue = "true")
protected boolean scanEndInclusive;

// Deprecated option - kept for backward compatibility
/**
* @deprecated As of release 3.17.0. Will be removed in release 4.0.0. Use --end-inclusive instead
* (inverted logic).
*/
@CommandLine.Option(
names = {DEPRECATED_END_EXCLUSIVE_OPTION},
description = "Deprecated: Use --end-inclusive instead (inverted logic)",
Expand Down
8 changes: 4 additions & 4 deletions ...i/src/main/java/com/scalar/db/dataloader/cli/command/dataimport/ImportCommandOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public class ImportCommandOptions {
protected Integer maxThreads;

/**
* @deprecated As of release 3.6.2. Will be removed in release 4.0.0. Use --max-threads instead
* @deprecated As of release 3.17.0. Will be removed in release 4.0.0. Use --max-threads instead
*/
@Deprecated
@CommandLine.Option(
Expand Down Expand Up @@ -75,7 +75,7 @@ public class ImportCommandOptions {
protected String controlFilePath;

/**
* @deprecated As of release 3.6.2. Will be removed in release 4.0.0. Use --enable-log-success
* @deprecated As of release 3.17.0. Will be removed in release 4.0.0. Use --enable-log-success
* instead
*/
@Deprecated
Expand Down Expand Up @@ -125,8 +125,8 @@ public class ImportCommandOptions {

@CommandLine.Option(
names = {"--log-raw-record", "-lr"},
description = "Include the original source record in the log file output (default: false)",
defaultValue = "false")
description = "Include the original source record in the log file output (default: true)",
defaultValue = "true")
Comment on lines +128 to +129
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

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

high

I have a couple of points regarding this change:

  • Inconsistency with Core Options: While changing the default value here to true aligns with the PR's goal, it creates an inconsistency with the core ImportOptions class, where logRawRecord still defaults to false. This means the default behavior will differ depending on whether the tool is used via the CLI or programmatically. For consistency, please consider also updating the default in data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/ImportOptions.java.

  • Missing Test Coverage: Changing a default value is a behavioral change that should be covered by tests. ImportCommandTest.java appears to be missing tests for the --log-raw-record option. Please add a test case to verify that logRawRecord defaults to true when the option is not specified in the command arguments.

Copy link
Contributor Author

@inv-jishnu inv-jishnu Nov 19, 2025

Choose a reason for hiding this comment

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

I have updated changes based on suggestion in 58c5051

protected boolean logRawRecord;

@CommandLine.Option(
Expand Down