You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "liuxiaocs7 (via GitHub)" <gi...@apache.org> on 2023/04/30 12:43:53 UTC
[GitHub] [iceberg] liuxiaocs7 opened a new pull request, #7481: Flink: Remove deprecated AssertHelpers
liuxiaocs7 opened a new pull request, #7481:
URL: https://github.com/apache/iceberg/pull/7481
- SubTask of https://github.com/apache/iceberg/issues/7094
Remove AssertHelpers in `iceberg-flink` module
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liuxiaocs7 commented on a diff in pull request #7481: Flink: Remove deprecated AssertHelpers
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on code in PR #7481:
URL: https://github.com/apache/iceberg/pull/7481#discussion_r1182110918
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -378,16 +371,14 @@ public void testOverrideWriteConfigWithUnknownFileFormat() {
.writeParallelism(parallelism)
.setAll(newProps);
- AssertHelpers.assertThrows(
- "Should fail with invalid file format.",
- IllegalArgumentException.class,
- "Invalid file format: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ builder.append();
+
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liuxiaocs7 commented on a diff in pull request #7481: Flink: Remove deprecated AssertHelpers
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on code in PR #7481:
URL: https://github.com/apache/iceberg/pull/7481#discussion_r1182110383
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,17 +345,15 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.writeParallelism(parallelism)
.setAll(newProps);
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ builder.append();
+
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
Review Comment:
got it, thanks!
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,17 +345,15 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.writeParallelism(parallelism)
.setAll(newProps);
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ builder.append();
+
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
Review Comment:
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7481: Flink: Remove deprecated AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7481:
URL: https://github.com/apache/iceberg/pull/7481#discussion_r1181486897
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -378,16 +371,14 @@ public void testOverrideWriteConfigWithUnknownFileFormat() {
.writeParallelism(parallelism)
.setAll(newProps);
- AssertHelpers.assertThrows(
- "Should fail with invalid file format.",
- IllegalArgumentException.class,
- "Invalid file format: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ builder.append();
+
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
Review Comment:
I think we should remove the call to `env.execute("Test Iceberg DataStream.");` because it implies that the error happens on that call, whereas in fact it happens when calling `builder.append()`
```
Assertions.assertThatThrownBy(builder::append)
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid file format: UNRECOGNIZED");
```
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,17 +345,15 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.writeParallelism(parallelism)
.setAll(newProps);
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ builder.append();
+
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
Review Comment:
I think we should remove the call to `env.execute("Test Iceberg DataStream.");` because it implies that the error happens on that call, whereas in fact it happens when calling `builder.append()`
```
Assertions.assertThatThrownBy(builder::append)
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid distribution mode: UNRECOGNIZED");
```
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liuxiaocs7 commented on a diff in pull request #7481: Flink: Remove deprecated AssertHelpers
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on code in PR #7481:
URL: https://github.com/apache/iceberg/pull/7481#discussion_r1182134619
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,21 +345,16 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.writeParallelism(parallelism)
.setAll(newProps);
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(builder::append)
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid distribution mode: UNRECOGNIZED");
+
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
Review Comment:
that's right, done!
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,21 +345,16 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.writeParallelism(parallelism)
.setAll(newProps);
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(builder::append)
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid distribution mode: UNRECOGNIZED");
+
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
Review Comment:
make sense, done!
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liuxiaocs7 commented on a diff in pull request #7481: Flink: Remove deprecated AssertHelpers
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on code in PR #7481:
URL: https://github.com/apache/iceberg/pull/7481#discussion_r1182134619
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,21 +345,16 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.writeParallelism(parallelism)
.setAll(newProps);
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(builder::append)
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid distribution mode: UNRECOGNIZED");
+
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
Review Comment:
done!
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liuxiaocs7 commented on pull request #7481: Flink: Remove deprecated AssertHelpers
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on PR #7481:
URL: https://github.com/apache/iceberg/pull/7481#issuecomment-1529059131
Hi, @nastra and @jackye1995, sorry for late, please take a look, 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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7481: Flink: Remove deprecated AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7481:
URL: https://github.com/apache/iceberg/pull/7481#discussion_r1182129831
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,21 +345,16 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.writeParallelism(parallelism)
.setAll(newProps);
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(builder::append)
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid distribution mode: UNRECOGNIZED");
+
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
Review Comment:
I think we don't need to call this at all because the exception happens on `builder::append` and we're only interested here about the detection of the `UNRECOGNIZED` distribution mode. So we can just remove this line here and in the test below
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra merged pull request #7481: Flink: Remove deprecated AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #7481:
URL: https://github.com/apache/iceberg/pull/7481
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org