You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zentol <gi...@git.apache.org> on 2016/06/08 15:12:20 UTC
[GitHub] flink pull request #2084: [FLINK-4032] Replace all usages Guava precondition...
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/2084
[FLINK-4032] Replace all usages Guava preconditions
This PR replaces every usage of the Guava Preconditions in Flink with our own Preconditions class.
In addition,
- the guava dependency was completely removed from the RabbitMQ connector
- a checkstyle rules was added preventing further use of guava preconditions
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink guava
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2084.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2084
----
commit 7bc5099e0fa7731b5fd8ab7d4a32e29c60468bc6
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:01:19Z
Remove guava dependency from flink-...-rabbitmq
commit 3f30f1f6bdd98b27ff815fbc56d8c9704e1091a6
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:01:49Z
Replace Preconditions usage in flink-..-elasticsearch2
commit 8e90a16ef75ed4adcd00261b326aa10f04f5ddac
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:25:26Z
Replace Preconditions usage in flink-table
commit cf61152d703abec9e2a47ecc102d2b148c172add
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:25:34Z
Replace Preconditions usage in flink-optimizer
commit b6e90150d3dda1b2cd2822e3031a604798f6dcaf
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:25:45Z
Replace Preconditions usage in flink-runtime-web
commit 910bf63778ba0a0b1b2ec183c66c524c3dd53ffc
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:25:54Z
Replace Preconditions usage in flink-scala
commit 9012b1dae48ef9aed50e6e2d9f8bd9c59c8abd80
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:25:58Z
Replace Preconditions usage in flink-yarn
commit 393da586a8ebcbca60d60b958aa21040ae5197d8
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:26:02Z
Replace Preconditions usage in flink-tests
commit 90723bf1defcd7baf72285b81c4c5732c8a25624
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:26:13Z
Replace Preconditions usage in flink-streaming-java
commit 5b27a648d363c1130fbff829c00a298b728d0ae7
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:26:25Z
Replace Preconditions usage in flink-runtime
commit c5ac8b21591e08ce74865d04baeffc344ad7867c
Author: zentol <ch...@apache.org>
Date: 2016-06-08T14:32:20Z
checkstyle rule
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2084: [FLINK-4032] Replace all usages Guava precondition...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2084
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2084: [FLINK-4032] Replace all usages Guava preconditions
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/2084
Ah right, sorry I was mistaken by the guava dependency in pom.xml. The dependency can actually be safely removed also though, must have accidentally left it there when the connector was cleaned up to use Flink's preconditions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2084: [FLINK-4032] Replace all usages Guava precondition...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/2084#discussion_r67119231
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/operators/util/BloomFilter.java ---
@@ -18,10 +18,10 @@
package org.apache.flink.runtime.operators.util;
-import com.google.common.base.Preconditions;
import org.apache.flink.core.memory.MemorySegment;
+import org.apache.flink.util.Preconditions;
--- End diff --
This import can be removed if the two `Preconditions.checkArgument` calls in the code are converted into `checkArgument` calls
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2084: [FLINK-4032] Replace all usages Guava precondition...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2084#discussion_r67120005
--- Diff: flink-core/src/main/java/org/apache/flink/util/Preconditions.java ---
@@ -234,6 +234,22 @@ public static void checkElementIndex(int index, int size) {
}
}
+ /**
+ * Ensures that the given index is valid for an array, list or string of the given size.
+ *
+ * @param index index to check
+ * @param size size of the array, list or string
+ *
+ * @throws IllegalArgumentException Thrown, if size is negative.
+ * @throws IndexOutOfBoundsException Thrown, if the index negative or greater than or equal to size
+ */
+ public static void checkElementIndex(int index, int size, @Nullable String errorMessage) {
+ checkArgument(size >= 0, "Size was negative.");
+ if (index < 0 || index >= size) {
+ throw new IndexOutOfBoundsException(errorMessage + " Index: " + index + ", Size: " + size);
--- End diff --
other Preconditions methods print also print "null" if no error message was given. granted, they use String.valueOf(errorMessage), but it's essentially the same.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2084: [FLINK-4032] Replace all usages Guava precondition...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/2084#discussion_r67119642
--- Diff: flink-core/src/main/java/org/apache/flink/util/Preconditions.java ---
@@ -234,6 +234,22 @@ public static void checkElementIndex(int index, int size) {
}
}
+ /**
+ * Ensures that the given index is valid for an array, list or string of the given size.
+ *
+ * @param index index to check
+ * @param size size of the array, list or string
--- End diff --
`errorMessage` parameter missing
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2084: [FLINK-4032] Replace all usages Guava preconditions
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/2084
I think the Kinesis connector is missed out as part of this cleanup. Thanks for this!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2084: [FLINK-4032] Replace all usages Guava preconditions
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/2084
Oops, my mistake again ;) Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2084: [FLINK-4032] Replace all usages Guava precondition...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/2084#discussion_r67118763
--- Diff: flink-core/src/main/java/org/apache/flink/util/Preconditions.java ---
@@ -234,6 +234,22 @@ public static void checkElementIndex(int index, int size) {
}
}
+ /**
+ * Ensures that the given index is valid for an array, list or string of the given size.
+ *
+ * @param index index to check
+ * @param size size of the array, list or string
+ *
+ * @throws IllegalArgumentException Thrown, if size is negative.
+ * @throws IndexOutOfBoundsException Thrown, if the index negative or greater than or equal to size
+ */
+ public static void checkElementIndex(int index, int size, @Nullable String errorMessage) {
+ checkArgument(size >= 0, "Size was negative.");
+ if (index < 0 || index >= size) {
+ throw new IndexOutOfBoundsException(errorMessage + " Index: " + index + ", Size: " + size);
--- End diff --
shouldn't we check if `errorMessage == null` if it is `@Nullable`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2084: [FLINK-4032] Replace all usages Guava preconditions
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2084
The Kinesis connector already uses Flink's Preconditions class.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2084: [FLINK-4032] Replace all usages Guava preconditions
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/2084
Thanks for the update @zentol. +1 to merge
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2084: [FLINK-4032] Replace all usages Guava precondition...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/2084#discussion_r67119824
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/FlinkYarnCluster.java ---
@@ -250,7 +250,7 @@ public void disconnect() {
*/
@Override
public void stopAfterJob(JobID jobID) {
- Preconditions.checkNotNull("The job id must not be null", jobID);
+ Preconditions.checkNotNull(jobID, "The job id must not be null");
--- End diff --
nice catch!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2084: [FLINK-4032] Replace all usages Guava preconditions
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/2084
Looks good overall. Added just a few minor comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2084: [FLINK-4032] Replace all usages Guava preconditions
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2084
It is still used in FlinkKinesisProducer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2084: [FLINK-4032] Replace all usages Guava preconditions
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2084
Merging
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2084: [FLINK-4032] Replace all usages Guava precondition...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/2084#discussion_r67146087
--- Diff: flink-core/src/main/java/org/apache/flink/util/Preconditions.java ---
@@ -234,6 +234,22 @@ public static void checkElementIndex(int index, int size) {
}
}
+ /**
+ * Ensures that the given index is valid for an array, list or string of the given size.
+ *
+ * @param index index to check
+ * @param size size of the array, list or string
+ *
+ * @throws IllegalArgumentException Thrown, if size is negative.
+ * @throws IndexOutOfBoundsException Thrown, if the index negative or greater than or equal to size
+ */
+ public static void checkElementIndex(int index, int size, @Nullable String errorMessage) {
+ checkArgument(size >= 0, "Size was negative.");
+ if (index < 0 || index >= size) {
+ throw new IndexOutOfBoundsException(errorMessage + " Index: " + index + ", Size: " + size);
--- End diff --
OK
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2084: [FLINK-4032] Replace all usages Guava precondition...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/2084#discussion_r67119787
--- Diff: tools/maven/checkstyle.xml ---
@@ -60,6 +60,17 @@ under the License.
<property name="illegalPattern" value="true"/>
<property name="message" value="Use Guava Checks instead of Commons Validate. Please refer to the coding guidelines."/>
</module>
+ <!-- forbid the use of google.common.base.Preconditions -->
+ <module name="Regexp">
+ <property name="format" value="import com\.google\.common\.base\.Preconditions"/>
+ <property name="illegalPattern" value="true"/>
+ <property name="message" value="Use Flink's Preconditions instead of guava's Preconditions"/>
--- End diff --
`Guava` with capital `G`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2084: [FLINK-4032] Replace all usages Guava preconditions
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2084
@fhueske I've addressed your comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---