[SPARK-5666][streaming][MQTT streaming] some trivial fixes #4178

prabeesh · 2015-01-23T10:33:40Z

modified to adhere to accepted coding standards as pointed by @tdas in PR #3844

SparkQA · 2015-01-23T10:37:50Z

Test build #26020 has started for PR 4178 at commit 5857989.

This patch merges cleanly.

SparkQA · 2015-01-23T11:32:04Z

Test build #26020 has finished for PR 4178 at commit 5857989.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-01-23T11:32:08Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26020/
Test FAILed.

prabeesh · 2015-01-23T11:44:49Z

Jenkins, retest this please.

SparkQA · 2015-01-23T11:47:43Z

Test build #26022 has started for PR 4178 at commit 5857989.

This patch merges cleanly.

SparkQA · 2015-01-23T12:56:45Z

Test build #26022 has finished for PR 4178 at commit 5857989.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-01-23T12:56:49Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26022/
Test PASSed.

the hard coded '\tmp' may create garbage.

SparkQA · 2015-01-23T17:27:46Z

Test build #26030 has started for PR 4178 at commit 66919a3.

This patch merges cleanly.

SparkQA · 2015-01-23T18:38:14Z

Test build #26030 has finished for PR 4178 at commit 66919a3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-01-23T18:38:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26030/
Test PASSed.

dragos · 2015-02-03T14:24:05Z

examples/src/main/scala/org/apache/spark/examples/streaming/MQTTWordCount.scala

I don't see the point in swallowing this exception. Why not let it percolate, it will anyway print the stacktrace and stop the thread?

SparkQA · 2015-02-03T17:42:59Z

Test build #26659 has started for PR 4178 at commit cd57029.

This patch merges cleanly.

SparkQA · 2015-02-03T18:54:38Z

Test build #26659 has finished for PR 4178 at commit cd57029.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-03T18:54:43Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26659/
Test PASSed.

srowen · 2015-02-07T15:37:57Z

external/mqtt/src/main/scala/org/apache/spark/streaming/mqtt/MQTTInputDStream.scala

I don't know how strong the convention is here, but I would have left the individual imports in this case.

srowen · 2015-02-07T15:41:23Z

Generally I'm liking the cleanup here, with a few questions inline. I think this deserves a JIRA too.

srowen · 2015-02-09T10:15:57Z

This is just waiting on response to my comments. most of it is that some of the import movements put imports in the wrong place now. (Some are correct changes though.) I'm also concerned about the infinite loop if the queue is full.

SparkQA · 2015-02-09T18:37:52Z

Test build #27113 has started for PR 4178 at commit 0cc67bd.

This patch merges cleanly.

SparkQA · 2015-02-09T18:42:39Z

Test build #27115 has started for PR 4178 at commit b042ad2.

This patch merges cleanly.

SparkQA · 2015-02-10T17:57:55Z

Test build #27216 has started for PR 4178 at commit 35f2cc3.

This patch merges cleanly.

prabeesh · 2015-02-10T18:00:05Z

external/mqtt/src/main/scala/org/apache/spark/streaming/mqtt/MQTTInputDStream.scala

Actually which is the right import style. The below or existing ?

import org.eclipse.paho.client.mqttv3.{IMqttDeliveryToken, MqttCallback, MqttClient, MqttClientPersistence, MqttException, MqttMessage, MqttTopic}

I think the multiple import syntax is often used, especially where it's definitely shorter, but, I would not change it just to change it.

SparkQA · 2015-02-10T18:02:28Z

Test build #27217 has started for PR 4178 at commit ccc0765.

This patch merges cleanly.

SparkQA · 2015-02-10T18:49:08Z

Test build #27215 has finished for PR 4178 at commit 46f9619.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-10T18:49:12Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27215/
Test PASSed.

SparkQA · 2015-02-10T19:14:05Z

Test build #27216 has finished for PR 4178 at commit 35f2cc3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-10T19:14:10Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27216/
Test PASSed.

SparkQA · 2015-02-10T19:26:31Z

Test build #27217 has finished for PR 4178 at commit ccc0765.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-10T19:26:34Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27217/
Test PASSed.

prabeesh · 2015-02-16T18:37:37Z

@srowen is there any more updates here?

srowen · 2015-02-16T18:49:53Z

@prabeesh this looks like it's waiting on a change to address the comment from @dragos - no need to catch and log the exception - and to the issue of infinite looping if the queue is full. I think it needs at least a warning that this is occurring. Yes it's an example that a 'real' deployment would have to change anyway, but I think it's worth showing good practice. Right now the error is always swallowed.

SparkQA · 2015-02-24T19:12:37Z

Test build #27897 has started for PR 4178 at commit bba0d67.

This patch merges cleanly.

SparkQA · 2015-02-24T19:13:42Z

Test build #27897 has finished for PR 4178 at commit bba0d67.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-24T19:13:43Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27897/
Test FAILed.

SparkQA · 2015-02-24T19:23:03Z

Test build #27899 has started for PR 4178 at commit bd2cb49.

This patch merges cleanly.

SparkQA · 2015-02-24T20:41:29Z

Test build #27899 has finished for PR 4178 at commit bd2cb49.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-24T20:41:33Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27899/
Test PASSed.

prabeesh · 2015-02-25T11:44:52Z

@srowen warning that what is occurring.

srowen · 2015-02-25T11:54:50Z

@prabeesh eh OK you mean you addressed that comment? I still am not sure that the comment from @dragos was addressed as I mentioned before. But since that catch block was already there, may not matter much. The cleanup is minor and in a part of code unlikely to be hit by merge conflicts. I think the benefit is really slightly better resource management in this example, and handling a full queue. I'll merge soon if nobody objects.

prabeesh · 2015-02-25T12:01:25Z

@srowen I think so I addressed the @dragos comments. Should we need more updates for that comments ?, If so could you please explain in detail ?

dragos · 2015-02-25T14:24:44Z

If my comment is the only thing that was holding this PR from merging, I withdraw my comment. :)

prabeesh · 2015-02-25T14:29:37Z

@dragos no need of withdraw the comment.
Ultimately my aim to deliver good code to Spark users.

modified to adhere to accepted coding standards

5857989

changed MqttDefaultFilePersistence to MemoryPersistence

66919a3

the hard coded '\tmp' may create garbage.

dragos reviewed Feb 3, 2015
View reviewed changes

address the comments

cd57029

prabeesh force-pushed the master branch from 66a51f7 to cd57029 Compare February 6, 2015 05:30

srowen reviewed Feb 7, 2015
View reviewed changes

prabeesh changed the title ~~[Minor][streaming][MQTT streaming] some trivial fixes~~ [SPARK-5666][streaming][MQTT streaming] some trivial fixes Feb 7, 2015

prabeesh added 2 commits February 10, 2015 00:03

adress the comment

838c38e

adress the comment

0cc67bd

adress the comment

ccc0765

prabeesh force-pushed the master branch from 35f2cc3 to ccc0765 Compare February 10, 2015 17:57

prabeesh reviewed Feb 10, 2015
View reviewed changes

adress the comment

bd2cb49

prabeesh force-pushed the master branch from bba0d67 to bd2cb49 Compare February 24, 2015 19:19

asfgit closed this in d51ed26 Feb 25, 2015

[SPARK-5666][streaming][MQTT streaming] some trivial fixes #4178

[SPARK-5666][streaming][MQTT streaming] some trivial fixes #4178

Uh oh!

Conversation

prabeesh commented Jan 23, 2015

Uh oh!

SparkQA commented Jan 23, 2015

Uh oh!

SparkQA commented Jan 23, 2015

Uh oh!

AmplabJenkins commented Jan 23, 2015

Uh oh!

prabeesh commented Jan 23, 2015

Uh oh!

SparkQA commented Jan 23, 2015

Uh oh!

SparkQA commented Jan 23, 2015

Uh oh!

AmplabJenkins commented Jan 23, 2015

Uh oh!

SparkQA commented Jan 23, 2015

Uh oh!

SparkQA commented Jan 23, 2015

Uh oh!

AmplabJenkins commented Jan 23, 2015

Uh oh!

dragos Feb 3, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 3, 2015

Uh oh!

SparkQA commented Feb 3, 2015

Uh oh!

AmplabJenkins commented Feb 3, 2015

Uh oh!

srowen Feb 7, 2015

Choose a reason for hiding this comment

Uh oh!

srowen commented Feb 7, 2015

Uh oh!

srowen commented Feb 9, 2015

Uh oh!

SparkQA commented Feb 9, 2015

Uh oh!

SparkQA commented Feb 9, 2015

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

prabeesh Feb 10, 2015

Choose a reason for hiding this comment

Uh oh!

srowen Feb 10, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

AmplabJenkins commented Feb 10, 2015

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

AmplabJenkins commented Feb 10, 2015

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

AmplabJenkins commented Feb 10, 2015

Uh oh!

prabeesh commented Feb 16, 2015

Uh oh!

srowen commented Feb 16, 2015

Uh oh!

SparkQA commented Feb 24, 2015

Uh oh!

SparkQA commented Feb 24, 2015

Uh oh!

AmplabJenkins commented Feb 24, 2015

Uh oh!

SparkQA commented Feb 24, 2015

Uh oh!