Skip to content

Core: Fix the incorrectly log description in BinPackStrategy#3919

Merged
rdblue merged 3 commits intoapache:masterfrom
Zhangg7723:fix_log_error
Jan 24, 2022
Merged

Core: Fix the incorrectly log description in BinPackStrategy#3919
rdblue merged 3 commits intoapache:masterfrom
Zhangg7723:fix_log_error

Conversation

@Zhangg7723
Copy link

This PR fixes a log error for rewrite parameter max-file-size-bytes, when I set the target-file-size-bytes less than max-file-size-bytes, spark throwed a checkAgument exception "Cannot set max-file-size-bytes is greater than or equal to target-file-size-bytes, all files written will be larger than the threshold", it's puzzling, so I fix this error.

@github-actions github-actions bot added the core label Jan 18, 2022
MIN_FILE_SIZE_BYTES, RewriteDataFiles.TARGET_FILE_SIZE_BYTES, minFileSize, targetFileSize);

Preconditions.checkArgument(targetFileSize < maxFileSize,
"Cannot set %s is greater than or equal to %s, all files written will be larger than the threshold, %d >= %d",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the correct fix is to reverse MAX_FILE_SIZE_BYTES and RewriteDataFiles.TARGET_FILE_SIZE_BYTES. Leaving "or equal to" doesn't work.

Copy link
Author

Choose a reason for hiding this comment

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

If MAX_FILE_SIZE_BYTES equals to TARGET_FILE_SIZE_BYTES, argument check still works and throws this exception. Anyway, there is a mistake I forget to fix "%d >= %d" with parameters, I will reverse targetFileSize and maxFileSize later.
thanks for your comment

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is incorrect for the equals case. Can you revert the message changes and swap the argument order?

Copy link
Author

Choose a reason for hiding this comment

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

OK, I will follow your advice, swap the argument order.

@rdblue
Copy link
Contributor

rdblue commented Jan 23, 2022

Thanks, @Zhangg7723! Looks good. I'll merge when tests are passing.

@rdblue rdblue merged commit 562e2b0 into apache:master Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants