You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/06/13 11:48:25 UTC
[GitHub] [flink] zhoulii opened a new pull request, #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
zhoulii opened a new pull request, #19946:
URL: https://github.com/apache/flink/pull/19946
## What is the purpose of the change
- correct the start index for creating empty splits in BinaryInputFormat#createInputSplits.
## Verifying this change
- Covered by unit test.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / **no** / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (**not applicable** / docs / JavaDocs / not documented)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #19946:
URL: https://github.com/apache/flink/pull/19946#issuecomment-1153819861
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "799139190d3647cea0935b3c12f4e759a0768f3f",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "799139190d3647cea0935b3c12f4e759a0768f3f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 799139190d3647cea0935b3c12f4e759a0768f3f UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhoulii commented on pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
zhoulii commented on PR #19946:
URL: https://github.com/apache/flink/pull/19946#issuecomment-1154791356
> > Maybe add a hotfix commit "Rework BinaryInputFormatTest to be based on AssertJ" first. And then add the new test using AssertJ.
>
> same as @zhuzhurk's previous suggestion, you should put the hotfix in the first commit, and then commit your changes and new tests follow it.
Sorry, I don't quite understand this suggestion. Do you mean I should start a new PR to Rework BinaryInputFormatTest first?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhuzhurk merged pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
zhuzhurk merged PR #19946:
URL: https://github.com/apache/flink/pull/19946
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhuzhurk commented on a diff in pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
zhuzhurk commented on code in PR #19946:
URL: https://github.com/apache/flink/pull/19946#discussion_r896402127
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -160,11 +161,45 @@ public void testGetStatisticsMultiplePaths() throws IOException {
stats.getTotalInputSize());
}
+ @Test
+ public void testCreateInputSplitsWithEmptySplit() throws IOException {
+ final int blockInfoSize = new BlockInfo().getInfoSize();
+ final int blockSize = blockInfoSize + 8;
+ final int numBlocks = 3;
+ final int minNumSplits = 5;
+
+ // create temporary file with 3 blocks
+ final File tempFile =
+ createBinaryInputFile(
+ "test_create_input_splits_with_empty_split", blockSize, numBlocks);
+
+ final Configuration config = new Configuration();
+ config.setLong("input.block_size", blockSize + 10);
+
+ final BinaryInputFormat<Record> inputFormat = new MyBinaryInputFormat();
+ inputFormat.setFilePath(tempFile.toURI().toString());
+ inputFormat.setBlockSize(blockSize);
+
+ inputFormat.configure(config);
+
+ FileInputSplit[] inputSplits = inputFormat.createInputSplits(minNumSplits);
+
+ Assert.assertEquals(
Review Comment:
Makes sense. Ref: https://flink.apache.org/contributing/code-style-and-quality-common.html#testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #19946:
URL: https://github.com/apache/flink/pull/19946#discussion_r896430817
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -154,17 +170,60 @@ public void testGetStatisticsMultiplePaths() throws IOException {
inputFormat.setBlockSize(blockSize);
BaseStatistics stats = inputFormat.getStatistics(null);
- Assert.assertEquals(
- "The file size statistics is wrong",
- blockSize * (numBlocks1 + numBlocks2),
- stats.getTotalInputSize());
+
+ assertThat(stats.getTotalInputSize())
+ .as("The file size statistics is wrong")
+ .isEqualTo(blockSize * (numBlocks1 + numBlocks2));
+ }
+
+ @Test
+ public void testCreateInputSplitsWithEmptySplit() throws IOException {
+ final int blockInfoSize = new BlockInfo().getInfoSize();
+ final int blockSize = blockInfoSize + 8;
+ final int numBlocks = 3;
+ final int minNumSplits = 5;
+
+ // create temporary file with 3 blocks
+ final File tempFile =
+ createBinaryInputFile(
+ "test_create_input_splits_with_empty_split", blockSize, numBlocks);
+
+ final Configuration config = new Configuration();
+ config.setLong("input.block_size", blockSize + 10);
+
+ final BinaryInputFormat<Record> inputFormat = new MyBinaryInputFormat();
+ inputFormat.setFilePath(tempFile.toURI().toString());
+ inputFormat.setBlockSize(blockSize);
+
+ inputFormat.configure(config);
+
+ FileInputSplit[] inputSplits = inputFormat.createInputSplits(minNumSplits);
+
+ assertThat(inputSplits.length)
+ .as("Returns requested numbers of splits.")
+ .isEqualTo(minNumSplits);
+
+ assertThat(inputSplits[0].getLength())
+ .as("1. split has block size length.")
Review Comment:
IMO, `as` is used to add description when this assert is not pass.
The error message will be displayed in the following format:
org.opentest4j.AssertionFailedError: [description in as()]
So I think the `as` description in these tests should be more accurate, such as
"1. split should/must has block size length."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #19946:
URL: https://github.com/apache/flink/pull/19946#discussion_r896438515
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -73,13 +76,21 @@ public void testCreateInputSplitsWithOneFile() throws IOException {
FileInputSplit[] inputSplits = inputFormat.createInputSplits(numBlocks);
- Assert.assertEquals("Returns requested numbers of splits.", numBlocks, inputSplits.length);
- Assert.assertEquals(
- "1. split has block size length.", blockSize, inputSplits[0].getLength());
- Assert.assertEquals(
- "2. split has block size length.", blockSize, inputSplits[1].getLength());
- Assert.assertEquals(
- "3. split has block size length.", blockSize, inputSplits[2].getLength());
+ assertThat(inputSplits.length)
Review Comment:
The same problem of other tests also needs to be modified
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhuzhurk commented on a diff in pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
zhuzhurk commented on code in PR #19946:
URL: https://github.com/apache/flink/pull/19946#discussion_r896403096
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -160,11 +161,45 @@ public void testGetStatisticsMultiplePaths() throws IOException {
stats.getTotalInputSize());
}
+ @Test
+ public void testCreateInputSplitsWithEmptySplit() throws IOException {
+ final int blockInfoSize = new BlockInfo().getInfoSize();
+ final int blockSize = blockInfoSize + 8;
+ final int numBlocks = 3;
+ final int minNumSplits = 5;
+
+ // create temporary file with 3 blocks
+ final File tempFile =
+ createBinaryInputFile(
+ "test_create_input_splits_with_empty_split", blockSize, numBlocks);
+
+ final Configuration config = new Configuration();
+ config.setLong("input.block_size", blockSize + 10);
+
+ final BinaryInputFormat<Record> inputFormat = new MyBinaryInputFormat();
+ inputFormat.setFilePath(tempFile.toURI().toString());
+ inputFormat.setBlockSize(blockSize);
+
+ inputFormat.configure(config);
+
+ FileInputSplit[] inputSplits = inputFormat.createInputSplits(minNumSplits);
+
+ Assert.assertEquals(
Review Comment:
Maybe add a hotfix commit "Rework BinaryInputFormatTest to be based on AssertJ" first. And then add the new test using AssertJ.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhuzhurk commented on pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
zhuzhurk commented on PR #19946:
URL: https://github.com/apache/flink/pull/19946#issuecomment-1154817846
> > > Maybe add a hotfix commit "Rework BinaryInputFormatTest to be based on AssertJ" first. And then add the new test using AssertJ.
> >
> >
> > same as @zhuzhurk's previous suggestion, you should put the hotfix in the first commit, and then commit your changes and new tests follow it.
>
> Sorry, I don't quite understand this suggestion. Do you mean I should start a new PR to Rework BinaryInputFormatTest first?
I mean that the hotfix commit happens first, and the commit of FLINK-28018 happens later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
reswqa commented on PR #19946:
URL: https://github.com/apache/flink/pull/19946#issuecomment-1154783029
> Maybe add a hotfix commit "Rework BinaryInputFormatTest to be based on AssertJ" first. And then add the new test using AssertJ.
same as @zhuzhurk's previous suggestion, you should put the hotfix in the first commit, and then commit your changes and new tests follow it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #19946:
URL: https://github.com/apache/flink/pull/19946#discussion_r896360739
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -160,11 +161,45 @@ public void testGetStatisticsMultiplePaths() throws IOException {
stats.getTotalInputSize());
}
+ @Test
+ public void testCreateInputSplitsWithEmptySplit() throws IOException {
+ final int blockInfoSize = new BlockInfo().getInfoSize();
+ final int blockSize = blockInfoSize + 8;
+ final int numBlocks = 3;
+ final int minNumSplits = 5;
+
+ // create temporary file with 3 blocks
+ final File tempFile =
+ createBinaryInputFile(
+ "test_create_input_splits_with_empty_split", blockSize, numBlocks);
+
+ final Configuration config = new Configuration();
+ config.setLong("input.block_size", blockSize + 10);
+
+ final BinaryInputFormat<Record> inputFormat = new MyBinaryInputFormat();
+ inputFormat.setFilePath(tempFile.toURI().toString());
+ inputFormat.setBlockSize(blockSize);
+
+ inputFormat.configure(config);
+
+ FileInputSplit[] inputSplits = inputFormat.createInputSplits(minNumSplits);
+
+ Assert.assertEquals(
Review Comment:
Are you willing to use assertj instead of junit.Assert,so that subsequent migration to junit5 will be more convenient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhoulii commented on a diff in pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
zhoulii commented on code in PR #19946:
URL: https://github.com/apache/flink/pull/19946#discussion_r896417184
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -160,11 +161,45 @@ public void testGetStatisticsMultiplePaths() throws IOException {
stats.getTotalInputSize());
}
+ @Test
+ public void testCreateInputSplitsWithEmptySplit() throws IOException {
+ final int blockInfoSize = new BlockInfo().getInfoSize();
+ final int blockSize = blockInfoSize + 8;
+ final int numBlocks = 3;
+ final int minNumSplits = 5;
+
+ // create temporary file with 3 blocks
+ final File tempFile =
+ createBinaryInputFile(
+ "test_create_input_splits_with_empty_split", blockSize, numBlocks);
+
+ final Configuration config = new Configuration();
+ config.setLong("input.block_size", blockSize + 10);
+
+ final BinaryInputFormat<Record> inputFormat = new MyBinaryInputFormat();
+ inputFormat.setFilePath(tempFile.toURI().toString());
+ inputFormat.setBlockSize(blockSize);
+
+ inputFormat.configure(config);
+
+ FileInputSplit[] inputSplits = inputFormat.createInputSplits(minNumSplits);
+
+ Assert.assertEquals(
Review Comment:
Thanks for reviewing, the suggestion is very helpful. I have reworked BinaryInputFormatTest to be based on AssertJ, can you take a look when you are free？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhoulii commented on pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
zhoulii commented on PR #19946:
URL: https://github.com/apache/flink/pull/19946#issuecomment-1155016277
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #19946:
URL: https://github.com/apache/flink/pull/19946#discussion_r896422901
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -73,13 +76,21 @@ public void testCreateInputSplitsWithOneFile() throws IOException {
FileInputSplit[] inputSplits = inputFormat.createInputSplits(numBlocks);
- Assert.assertEquals("Returns requested numbers of splits.", numBlocks, inputSplits.length);
- Assert.assertEquals(
- "1. split has block size length.", blockSize, inputSplits[0].getLength());
- Assert.assertEquals(
- "2. split has block size length.", blockSize, inputSplits[1].getLength());
- Assert.assertEquals(
- "3. split has block size length.", blockSize, inputSplits[2].getLength());
+ assertThat(inputSplits.length)
Review Comment:
```suggestion
assertThat(inputSplits)
.as("Returns requested numbers of splits.")
.hasSize(numBlocks);
```
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -154,17 +170,60 @@ public void testGetStatisticsMultiplePaths() throws IOException {
inputFormat.setBlockSize(blockSize);
BaseStatistics stats = inputFormat.getStatistics(null);
- Assert.assertEquals(
- "The file size statistics is wrong",
- blockSize * (numBlocks1 + numBlocks2),
- stats.getTotalInputSize());
+
+ assertThat(stats.getTotalInputSize())
+ .as("The file size statistics is wrong")
+ .isEqualTo(blockSize * (numBlocks1 + numBlocks2));
+ }
+
+ @Test
+ public void testCreateInputSplitsWithEmptySplit() throws IOException {
+ final int blockInfoSize = new BlockInfo().getInfoSize();
+ final int blockSize = blockInfoSize + 8;
+ final int numBlocks = 3;
+ final int minNumSplits = 5;
+
+ // create temporary file with 3 blocks
+ final File tempFile =
+ createBinaryInputFile(
+ "test_create_input_splits_with_empty_split", blockSize, numBlocks);
+
+ final Configuration config = new Configuration();
+ config.setLong("input.block_size", blockSize + 10);
+
+ final BinaryInputFormat<Record> inputFormat = new MyBinaryInputFormat();
+ inputFormat.setFilePath(tempFile.toURI().toString());
+ inputFormat.setBlockSize(blockSize);
+
+ inputFormat.configure(config);
+
+ FileInputSplit[] inputSplits = inputFormat.createInputSplits(minNumSplits);
+
+ assertThat(inputSplits.length)
+ .as("Returns requested numbers of splits.")
+ .isEqualTo(minNumSplits);
+
+ assertThat(inputSplits[0].getLength())
+ .as("1. split has block size length.")
+ .isEqualTo(blockSize);
+
+ assertThat(inputSplits[1].getLength())
+ .as("2. split has block size length.")
+ .isEqualTo(blockSize);
+
+ assertThat(inputSplits[2].getLength())
+ .as("3. split has block size length.")
+ .isEqualTo(blockSize);
+
+ assertThat(inputSplits[3].getLength()).as("4. split has block size length.").isEqualTo(0);
+
+ assertThat(inputSplits[4].getLength()).as("5. split has block size length.").isEqualTo(0);
Review Comment:
```suggestion
assertThat(inputSplits[4].getLength()).as("5. split should be an empty split.").isZero();
```
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -154,17 +170,60 @@ public void testGetStatisticsMultiplePaths() throws IOException {
inputFormat.setBlockSize(blockSize);
BaseStatistics stats = inputFormat.getStatistics(null);
- Assert.assertEquals(
- "The file size statistics is wrong",
- blockSize * (numBlocks1 + numBlocks2),
- stats.getTotalInputSize());
+
+ assertThat(stats.getTotalInputSize())
+ .as("The file size statistics is wrong")
+ .isEqualTo(blockSize * (numBlocks1 + numBlocks2));
+ }
+
+ @Test
+ public void testCreateInputSplitsWithEmptySplit() throws IOException {
+ final int blockInfoSize = new BlockInfo().getInfoSize();
+ final int blockSize = blockInfoSize + 8;
+ final int numBlocks = 3;
+ final int minNumSplits = 5;
+
+ // create temporary file with 3 blocks
+ final File tempFile =
+ createBinaryInputFile(
+ "test_create_input_splits_with_empty_split", blockSize, numBlocks);
+
+ final Configuration config = new Configuration();
+ config.setLong("input.block_size", blockSize + 10);
+
+ final BinaryInputFormat<Record> inputFormat = new MyBinaryInputFormat();
+ inputFormat.setFilePath(tempFile.toURI().toString());
+ inputFormat.setBlockSize(blockSize);
+
+ inputFormat.configure(config);
+
+ FileInputSplit[] inputSplits = inputFormat.createInputSplits(minNumSplits);
+
+ assertThat(inputSplits.length)
+ .as("Returns requested numbers of splits.")
+ .isEqualTo(minNumSplits);
+
+ assertThat(inputSplits[0].getLength())
+ .as("1. split has block size length.")
Review Comment:
IMO, `as` is used to add description when this assert is not pass.
The error message will be displayed in the following form:
org.opentest4j.AssertionFailedError: [description in as()]
So I think the `as` description in these tests should be more accurate, such as
"1. split should/must has block size length."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhuzhurk commented on a diff in pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
zhuzhurk commented on code in PR #19946:
URL: https://github.com/apache/flink/pull/19946#discussion_r896335558
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -160,6 +160,44 @@ public void testGetStatisticsMultiplePaths() throws IOException {
stats.getTotalInputSize());
}
+ @Test
+ public void testCreateInputSplitsWithEmptySplit() throws IOException {
+ // create temporary file with 3 blocks
+ final File tempFile = File.createTempFile("binary_input_format_test", "tmp");
+ tempFile.deleteOnExit();
Review Comment:
The `tempFile` can be deleted only if the whole JVM exits. I would suggest to use `TemporaryFolder` which can be cleaned after the test method finishes. `FlinkUserCodeClassLoadersTest` can be an example to reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhoulii commented on pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
zhoulii commented on PR #19946:
URL: https://github.com/apache/flink/pull/19946#issuecomment-1154820246
>
got it, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhoulii commented on a diff in pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
zhoulii commented on code in PR #19946:
URL: https://github.com/apache/flink/pull/19946#discussion_r896348640
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -160,6 +160,44 @@ public void testGetStatisticsMultiplePaths() throws IOException {
stats.getTotalInputSize());
}
+ @Test
+ public void testCreateInputSplitsWithEmptySplit() throws IOException {
+ // create temporary file with 3 blocks
+ final File tempFile = File.createTempFile("binary_input_format_test", "tmp");
+ tempFile.deleteOnExit();
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #19946: [FLINK-28018][core] correct the start index for creating empty splits in BinaryInputFormat#createInputSplits
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #19946:
URL: https://github.com/apache/flink/pull/19946#discussion_r896430817
##########
flink-core/src/test/java/org/apache/flink/api/common/io/BinaryInputFormatTest.java:
##########
@@ -154,17 +170,60 @@ public void testGetStatisticsMultiplePaths() throws IOException {
inputFormat.setBlockSize(blockSize);
BaseStatistics stats = inputFormat.getStatistics(null);
- Assert.assertEquals(
- "The file size statistics is wrong",
- blockSize * (numBlocks1 + numBlocks2),
- stats.getTotalInputSize());
+
+ assertThat(stats.getTotalInputSize())
+ .as("The file size statistics is wrong")
+ .isEqualTo(blockSize * (numBlocks1 + numBlocks2));
+ }
+
+ @Test
+ public void testCreateInputSplitsWithEmptySplit() throws IOException {
+ final int blockInfoSize = new BlockInfo().getInfoSize();
+ final int blockSize = blockInfoSize + 8;
+ final int numBlocks = 3;
+ final int minNumSplits = 5;
+
+ // create temporary file with 3 blocks
+ final File tempFile =
+ createBinaryInputFile(
+ "test_create_input_splits_with_empty_split", blockSize, numBlocks);
+
+ final Configuration config = new Configuration();
+ config.setLong("input.block_size", blockSize + 10);
+
+ final BinaryInputFormat<Record> inputFormat = new MyBinaryInputFormat();
+ inputFormat.setFilePath(tempFile.toURI().toString());
+ inputFormat.setBlockSize(blockSize);
+
+ inputFormat.configure(config);
+
+ FileInputSplit[] inputSplits = inputFormat.createInputSplits(minNumSplits);
+
+ assertThat(inputSplits.length)
+ .as("Returns requested numbers of splits.")
+ .isEqualTo(minNumSplits);
+
+ assertThat(inputSplits[0].getLength())
+ .as("1. split has block size length.")
Review Comment:
IMO, `as` is used to add description when this assert is not pass.
The error message will be displayed in the following format:
`org.opentest4j.AssertionFailedError: [description in as()] `
So I think the `as` description in these tests should be more accurate, such as
"1. split should/must has block size length."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org