You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by andralungu <gi...@git.apache.org> on 2015/02/26 00:40:49 UTC
[GitHub] flink pull request: [FLINK-1587][gelly] Added additional check for...
GitHub user andralungu opened a pull request:
https://github.com/apache/flink/pull/440
[FLINK-1587][gelly] Added additional check for edge src/trg id validity
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andralungu/flink exceptionBugFix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/440.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 #440
----
commit c36cf215ce68ae772a25333af99907730f4847d7
Author: andralungu <lu...@gmail.com>
Date: 2015-02-25T23:39:55Z
[FLINK-1587][gelly] Added additional check for edge src/trg id validity
----
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by vasia <gi...@git.apache.org>.
Github user vasia commented on the pull request:
https://github.com/apache/flink/pull/440#issuecomment-77687765
Thanks for the updates @andralungu. I'll merge this today :)
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by vasia <gi...@git.apache.org>.
Github user vasia commented on the pull request:
https://github.com/apache/flink/pull/440#issuecomment-77163740
Thanks Ufuk!
@andralungu, could you please change the test to use the logger?
@uce, regarding the exceptions output, I see this after having rebased on the latest master (double checked the commit you're referring to is in there..)
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/440#discussion_r25846311
--- Diff: flink-staging/flink-gelly/src/test/java/org/apache/flink/graph/test/DegreesWithExceptionITCase.java ---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.graph.test;
+
+import org.apache.flink.api.java.ExecutionEnvironment;
+import org.apache.flink.graph.Graph;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.NoSuchElementException;
+
+public class DegreesWithExceptionITCase {
+
+ @Test
+ public void testOutDegreesInvalidEdgeSrcId() throws Exception {
+ /*
+ * Test outDegrees() with an edge having a srcId that does not exist in the vertex DataSet
+ */
+ final ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
+
+ Graph<Long, Long, Long> graph = Graph.fromDataSet(TestGraphUtils.getLongLongVertexData(env),
+ TestGraphUtils.getLongLongEdgeInvalidSrcData(env), env);
+
+ try {
+ graph.outDegrees().print();
+ env.execute();
--- End diff --
If you expect always an exception here, then we should add a ```fail``` after the call. Otherwise the test would also pass if no exception was thrown.
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by vasia <gi...@git.apache.org>.
Github user vasia commented on the pull request:
https://github.com/apache/flink/pull/440#issuecomment-76399944
Thanks for the revision @andralungu! Looks good 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: [FLINK-1587][gelly] Added additional check for...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/440#discussion_r25846323
--- Diff: flink-staging/flink-gelly/src/test/java/org/apache/flink/graph/test/DegreesWithExceptionITCase.java ---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.graph.test;
+
+import org.apache.flink.api.java.ExecutionEnvironment;
+import org.apache.flink.graph.Graph;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.NoSuchElementException;
+
+public class DegreesWithExceptionITCase {
+
+ @Test
+ public void testOutDegreesInvalidEdgeSrcId() throws Exception {
+ /*
+ * Test outDegrees() with an edge having a srcId that does not exist in the vertex DataSet
+ */
+ final ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
+
+ Graph<Long, Long, Long> graph = Graph.fromDataSet(TestGraphUtils.getLongLongVertexData(env),
+ TestGraphUtils.getLongLongEdgeInvalidSrcData(env), env);
+
+ try {
+ graph.outDegrees().print();
+ env.execute();
+ } catch (Exception e) {
+ Assert.assertEquals("The edge src/trg id could not be found within the vertexIds", e.getCause().getMessage());
+ Assert.assertTrue(e.getCause() instanceof NoSuchElementException);
+ }
+ }
+
+ @Test
+ public void testInDegreesInvalidEdgeTrgId() throws Exception {
+ /*
+ * Test inDegrees() with an edge having a trgId that does not exist in the vertex DataSet
+ */
+ final ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
+
+ Graph<Long, Long, Long> graph = Graph.fromDataSet(TestGraphUtils.getLongLongVertexData(env),
+ TestGraphUtils.getLongLongEdgeInvalidTrgData(env), env);
+
+ try {
+ graph.inDegrees().print();
+ env.execute();
--- End diff --
same here
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the pull request:
https://github.com/apache/flink/pull/440#issuecomment-76914047
The failure in TaskManagerFailsITCase should be unrelated to your PR. This
should be fixed once the proper shading is in place. So don't worry and go
ahead merging it.
On Mon, Mar 2, 2015 at 8:55 PM, Vasia Kalavri <no...@github.com>
wrote:
> There is a failure in TaskManagerFailsITCase for Hadoop 2.0.0-alpha on
> travis.
> Is this fixed or something I can ignore and go ahead and merge this one?
> :))
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/flink/pull/440#issuecomment-76799370>.
>
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/440#discussion_r25846333
--- Diff: flink-staging/flink-gelly/src/test/java/org/apache/flink/graph/test/DegreesWithExceptionITCase.java ---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.graph.test;
+
+import org.apache.flink.api.java.ExecutionEnvironment;
+import org.apache.flink.graph.Graph;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.NoSuchElementException;
+
+public class DegreesWithExceptionITCase {
+
+ @Test
+ public void testOutDegreesInvalidEdgeSrcId() throws Exception {
+ /*
+ * Test outDegrees() with an edge having a srcId that does not exist in the vertex DataSet
+ */
+ final ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
+
+ Graph<Long, Long, Long> graph = Graph.fromDataSet(TestGraphUtils.getLongLongVertexData(env),
+ TestGraphUtils.getLongLongEdgeInvalidSrcData(env), env);
+
+ try {
+ graph.outDegrees().print();
+ env.execute();
+ } catch (Exception e) {
+ Assert.assertEquals("The edge src/trg id could not be found within the vertexIds", e.getCause().getMessage());
+ Assert.assertTrue(e.getCause() instanceof NoSuchElementException);
+ }
+ }
+
+ @Test
+ public void testInDegreesInvalidEdgeTrgId() throws Exception {
+ /*
+ * Test inDegrees() with an edge having a trgId that does not exist in the vertex DataSet
+ */
+ final ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
+
+ Graph<Long, Long, Long> graph = Graph.fromDataSet(TestGraphUtils.getLongLongVertexData(env),
+ TestGraphUtils.getLongLongEdgeInvalidTrgData(env), env);
+
+ try {
+ graph.inDegrees().print();
+ env.execute();
+ } catch (Exception e) {
+ Assert.assertEquals("The edge src/trg id could not be found within the vertexIds", e.getCause().getMessage());
+ Assert.assertTrue(e.getCause() instanceof NoSuchElementException);
+ }
+ }
+
+ @Test
+ public void testGetDegreesInvalidEdgeTrgId() throws Exception {
+ /*
+ * Test getDegrees() with an edge having a trgId that does not exist in the vertex DataSet
+ */
+ final ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
+
+ Graph<Long, Long, Long> graph = Graph.fromDataSet(TestGraphUtils.getLongLongVertexData(env),
+ TestGraphUtils.getLongLongEdgeInvalidTrgData(env), env);
+
+ try {
+ graph.getDegrees().print();
+ env.execute();
--- End diff --
I guess it's the same for all test cases.
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by vasia <gi...@git.apache.org>.
Github user vasia commented on the pull request:
https://github.com/apache/flink/pull/440#issuecomment-77004964
perfect, 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: [FLINK-1587][gelly] Added additional check for...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/440
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/440#issuecomment-77165497
Thanks, Vasia! :) It still seems to be a problem. I've re-opened the [corresponding issue](https://issues.apache.org/jira/browse/FLINK-1577) and will fix it. In any case, nothing should be wrong with the code here (it is a task cancellation race).
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by andralungu <gi...@git.apache.org>.
Github user andralungu commented on the pull request:
https://github.com/apache/flink/pull/440#issuecomment-77224411
Hi @uce!
I am using a data set's print() method in that particular test suite. I tried several mixes and matches with the logger but none were successful.
If, for example you do a log.info, that requires a string. How can you add the DataSet there? I tried adding it with .toString() in the end, but the problem is that, then, env.execute(), which expects a print or write in order to consume the data will no longer work.
Could you give me more details?
Thanks :)
Andra
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the pull request:
https://github.com/apache/flink/pull/440#issuecomment-77324311
Hi Andra, if you are using ```print()``` only because you need a sink in your job, then I think you could also use ```dataset.output(new DiscardingOutputFormat())```. This will simply discard all output.
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/440#discussion_r25476732
--- Diff: flink-staging/flink-gelly/src/test/java/org/apache/flink/graph/test/DegreesWithExceptionITCase.java ---
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.graph.test;
+
+import org.apache.flink.api.java.ExecutionEnvironment;
+import org.apache.flink.graph.Graph;
+import org.junit.Test;
+
+import java.util.NoSuchElementException;
+
+public class DegreesWithExceptionITCase {
+
+ @Test
+ public void testOutDegreesInvalidEdgeSrcId() throws Exception {
+ /*
+ * Test outDegrees() with an edge having a srcId that does not exist in the vertex DataSet
+ */
+ final ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
+
+ Graph<Long, Long, Long> graph = Graph.fromDataSet(TestGraphUtils.getLongLongVertexData(env),
+ TestGraphUtils.getLongLongEdgeInvalidSrcData(env), env);
+
+ try {
+ graph.outDegrees().print();
+ env.execute();
+ } catch (Exception e) {
+ assert e.getCause().getMessage().equals("The edge src/trg id could not be found within the vertexIds");
--- End diff --
I don't think we are using Java assertions in test cases.
We usually use junit assertions, such as `Assert.assertTrue()` in testcases.
I'm not even sure if assertions are always enabled when running tests through maven.
---
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: [FLINK-1587][gelly] Added additional check for...
Posted by vasia <gi...@git.apache.org>.
Github user vasia commented on the pull request:
https://github.com/apache/flink/pull/440#issuecomment-77068115
Hi,
I was about to merge this one, but I have two doubts:
- the tests print all execution output, including the exceptions, in standard output. Is this OK or should we avoid it?
- apart from the expected exception, the last 3 tests also produce the following:
```
java.lang.Exception: The data preparation for task 'Reduce(SUM(1), at getDegrees(Graph.java:664) (981992a0fbf442e4039eba434b173362)' , caused an error: Error obtaining the sorted input: Thread 'SortMerger Reading Thread' terminated due to an exception: Bug in input gate/channel logic: input gate gotnotified by channel about available data, but none was available.
at org.apache.flink.runtime.operators.RegularPactTask.run(RegularPactTask.java:471)
at org.apache.flink.runtime.operators.RegularPactTask.invoke(RegularPactTask.java:362)
at org.apache.flink.runtime.execution.RuntimeEnvironment.run(RuntimeEnvironment.java:205)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.RuntimeException: Error obtaining the sorted input: Thread 'SortMerger Reading Thread' terminated due to an exception: Bug in input gate/channel logic: input gate gotnotified by channel about available data, but none was available.
at org.apache.flink.runtime.operators.sort.UnilateralSortMerger.getIterator(UnilateralSortMerger.java:607)
at org.apache.flink.runtime.operators.RegularPactTask.getInput(RegularPactTask.java:1131)
at org.apache.flink.runtime.operators.GroupReduceDriver.prepare(GroupReduceDriver.java:94)
at org.apache.flink.runtime.operators.RegularPactTask.run(RegularPactTask.java:466)
... 3 more
Caused by: java.io.IOException: Thread 'SortMerger Reading Thread' terminated due to an exception: Bug in input gate/channel logic: input gate gotnotified by channel about available data, but none was available.
at org.apache.flink.runtime.operators.sort.UnilateralSortMerger$ThreadBase.run(UnilateralSortMerger.java:784)
Caused by: java.lang.IllegalStateException: Bug in input gate/channel logic: input gate gotnotified by channel about available data, but none was available.
at org.apache.flink.runtime.io.network.partition.consumer.SingleInputGate.getNextBufferOrEvent(SingleInputGate.java:314)
at org.apache.flink.runtime.io.network.partition.consumer.UnionInputGate.getNextBufferOrEvent(UnionInputGate.java:141)
at org.apache.flink.runtime.io.network.api.reader.AbstractRecordReader.getNextRecord(AbstractRecordReader.java:75)
at org.apache.flink.runtime.io.network.api.reader.MutableRecordReader.next(MutableRecordReader.java:34)
at org.apache.flink.runtime.operators.util.ReaderIterator.next(ReaderIterator.java:59)
at org.apache.flink.runtime.operators.sort.UnilateralSortMerger$ReadingThread.go(UnilateralSortMerger.java:958)
at org.apache.flink.runtime.operators.sort.UnilateralSortMerger$ThreadBase.run(UnilateralSortMerger.java:781)
```
I'm not sure where this is coming from and whether it's a problem in this case. Any ideas?
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: [FLINK-1587][gelly] Added additional check for...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/440#issuecomment-77125258
Hey Vasia,
1. I think the tests shouldn't print to stdout, but use the logger for test output.
2. The exceptions were wrong-fully logged for cancelled/failed task. I've fixed it yesterday in 9255594fb3b9b7c00d9088c3b630af9ecbdf22f4. Have you already rebased on the recent master?
– Ufuk
---
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.
---