Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-4295][External]Fix exception in SparkSinkSuite #3177

Closed
wants to merge 3 commits into from
Closed

[SPARK-4295][External]Fix exception in SparkSinkSuite #3177

wants to merge 3 commits into from

Conversation

maji2014
Copy link
Contributor

@maji2014 maji2014 commented Nov 9, 2014

Handle exception in SparkSinkSuite, please refer to [SPARK-4295]

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@srowen
Copy link
Member

srowen commented Nov 9, 2014

Can you clarify how the tests pass if an exception is thrown? does that also need a fix?

@maji2014
Copy link
Contributor Author

Exception throws before each test case although all test cases are passed . That's not the expect behavior i think. In order to avoid exception in each test case, set different channel name in each test case.
The behavior always appear when running SparkSinkSuite.scala directly
4/11/10 02:01:22 ERROR MonitoredCounterGroup: Failed to register monitored counter group for type: CHANNEL, name: null, javax.management.InstanceAlreadyExistsException: org.apache.flume.channel:type=null

@tdas
Copy link
Contributor

tdas commented Nov 11, 2014

@maji2014 Correct, I verified this error after adding a log4j properties in the external/flume-sink/src/test/resources and seeing the log4j logs. And manually verified that this test fixes the silent error.

As part of this patch, could you add the missing log4j.properties in external/flume-sink/src/test/resources, by copying it from streaming/src/test/resources ?

@harishreedharan you should take a look at this.

tdas
tdas reviewed Nov 11, 2014
View reviewed changes
external/flume-sink/src/test/scala/org/apache/spark/streaming/flume/sink/SparkSinkSuite.scala
@@ -159,6 +159,7 @@ class SparkSinkSuite extends FunSuite {
channelContext.put("transactionCapacity", 1000.toString)
channelContext.put("keep-alive", 0.toString)
channelContext.putAll(overrides)
channel.setName(getClass.getDeclaredMethods.toString)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this crazy name? Since this is being only used in this class, this string will always be the same. In that case, we can simple do "Channel", isnt it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simply use a randomly generated string as the channel name, so the InstanceAlreadyExistsException would not pop up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i have no good idea about how to get a random string name, such as how to get each test case name. In getClass.getDeclaredMethods.toString, we can get different string like[Ljava.lang.reflect.Method;@5f3ef269.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, that's nifty, but hugely non-intuitive. Rather, please use scala.util.Random.nextString(10)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please address the other comment about adding log4j.properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's ok

@tdas
Copy link
Contributor

tdas commented Nov 11, 2014

Jenkins, this is ok to test

@SparkQA
Copy link

SparkQA commented Nov 11, 2014

Test build #23212 has started for PR 3177 at commit 312620a.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Nov 11, 2014

Test build #23212 has finished for PR 3177 at commit 312620a.

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

@AmplabJenkins
Copy link

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

@tdas
Copy link
Contributor

tdas commented Nov 11, 2014

@maji2014 Thanks for this fix. Merging this.

asfgit pushed a commit that referenced this pull request Nov 11, 2014
@tdas
[SPARK-4295][External]Fix exception in SparkSinkSuite
bf867c3 
Handle exception in SparkSinkSuite, please refer to [SPARK-4295]

Author: maji2014 <[email protected]>

Closes #3177 from maji2014/spark-4295 and squashes the following commits:

312620a [maji2014] change a new statement for spark-4295
24c3d21 [maji2014] add log4j.properties for SparkSinkSuite and spark-4295
c807bf6 [maji2014] Fix exception in SparkSinkSuite

(cherry picked from commit f8811a5)
Signed-off-by: Tathagata Das <[email protected]>
asfgit pushed a commit that referenced this pull request Nov 11, 2014
@tdas
[SPARK-4295][External]Fix exception in SparkSinkSuite
fe8a1cd 
Handle exception in SparkSinkSuite, please refer to [SPARK-4295]

Author: maji2014 <[email protected]>

Closes #3177 from maji2014/spark-4295 and squashes the following commits:

312620a [maji2014] change a new statement for spark-4295
24c3d21 [maji2014] add log4j.properties for SparkSinkSuite and spark-4295
c807bf6 [maji2014] Fix exception in SparkSinkSuite

(cherry picked from commit f8811a5)
Signed-off-by: Tathagata Das <[email protected]>
@asfgit asfgit closed this in f8811a5 Nov 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maji2014 @AmplabJenkins @srowen @tdas @SparkQA @harishreedharan