-
Notifications
You must be signed in to change notification settings - Fork 28.6k
[SPARK-2103][Streaming] Change to ClassTag for KafkaInputDStream and fix reflection issue #1508
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.
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
Conversation
QA tests have started for PR 1508. This patch merges cleanly. |
Nice one Jerry! this actually enables using Kafka with non-String data in Java. |
QA results for PR 1508: |
It's the MIMA test that fails, since the method signature is changed. It's possible to keep and deprecate the existing method of course. Should we just do that, or OK to remove the method on the grounds that the API doesn't quite work? |
Jenkins, test this please. |
QA tests have started for PR 1508. This patch merges cleanly. |
LGTM. I am okay with the binary compatibility change, since it is really wrong that we were using manifest instead of classtags. But I am not sure I really understand what is the reason behind the failure of the |
Oh, please add the appropriate exclude in the Mima exclusions See the error in the jenkins console output for the exact exception string that you need to add. |
QA results for PR 1508: |
This failure is unrelated to this patch. Its a current known issue in Jenkins, will run the test again when this error gets cleared. |
Hi TD, thanks for your review. I think the previous issue is that: implicit val keyCmd: Manifest[U] = implicitly[Manifest[AnyRef]].asInstanceOf[Manifest[U]] This line of code cannot actually get the correct runtime class, the runtime class of Because we only got val keyDecoder = manifest[U].runtimeClass.getConstructor(classOf[VerifiableProperties]) The right way is like this: implicit val keyCmd: Manifest[U] = Manifest.classType(keyDecodeClass) Manifest can also do the same thing if we correctly feed it with class info. But I think it would be better to change Manifest to ClassTag to keep align with other code, so I changed to use ClassTag. I will add this to Mima exclude. Thanks a lot. |
I get it now. Let me try to run the Jenkins once again, to figure out what needs to be added to Mima |
Jenkins, test this please. |
QA tests have started for PR 1508. This patch merges cleanly. |
QA results for PR 1508: |
Again, this is an unrelated failure. Running it again. |
Jenkins, test this please. |
QA tests have started for PR 1508. This patch merges cleanly. |
QA results for PR 1508: |
Can you add these to the Mima excludes.
The last one should not be necessary as it is not a public class. Let me look into this. No harm in adding the exclusion though. |
QA tests have started for PR 1508. This patch DID NOT merge cleanly! |
@jerryshao Oops, not sure how that is possible given what this patch touches. Can you merge with master nonetheless. |
Ok, I will update the code. |
…ecoder construct issue when using Java API
QA tests have started for PR 1508. This patch merges cleanly. |
QA results for PR 1508: |
QA results for PR 1508: |
OK, this is very confusing, sequence of two results is very confusing. Let me run the tests again. |
Jenkins, test this again. |
Jenkins, test this please. |
QA tests have started for PR 1508. This patch merges cleanly. |
A bunch of tests have been failing spuriously with "java.net.BindException: Address already in use". It's not the PR. I wonder what recent change could have made this happen? Is something being stricter about assigning a fixed port? did a config change to let multiple tests run on the same virtual machine? |
Also, a number of them have been failing for spurious python mllib issues. At least was the case yesterday. |
QA results for PR 1508: |
Heyyyaa, it finally passed. I am merging this. Thanks @jerryshao |
…fix reflection issue This PR updates previous Manifest for KafkaInputDStream's Decoder to ClassTag, also fix the problem addressed in [SPARK-2103](https://issues.apache.org/jira/browse/SPARK-2103). Previous Java interface cannot actually get the type of Decoder, so when using this Manifest to reconstruct the decode object will meet reflection exception. Also for other two Java interfaces, ClassTag[String] is useless because calling Scala API will get the right implicit ClassTag. Current Kafka unit test cannot actually verify the interface. I've tested these interfaces in my local and distribute settings. Author: jerryshao <saisai.shao@intel.com> Closes apache#1508 from jerryshao/SPARK-2103 and squashes the following commits: e90c37b [jerryshao] Add Mima excludes 7529810 [jerryshao] Change Manifest to ClassTag for KafkaInputDStream's Decoder and fix Decoder construct issue when using Java API
I'm not sure if this is the exactly same issue, but I am experience this with Spark 1.2.1 and Scala 2.10.4 also.
|
Hi @salex89 , seems your exception is a little different to this PR, maybe I need a careful look at this exception, thanks for your reporting. |
@jerryshao |
Yeah, please do it and describe it in detail so we can easily reproduce it. |
…locks.enabled` by default (apache#1508) ### What changes were proposed in this pull request? rdar://99068608 (SPARK-40198 Enable `spark.storage.decommission.(rdd|shuffle)Blocks.enabled` by default) This PR aims to enable the following two configurations by default. Note that these storage configurations are disabled by default by the parent configuration, `spark.storage.decommission.enabled`, still. In addition, all decommission features including this storage decommissioning are disabled by `spark.decommission.enabled` by default. - `spark.storage.decommission.rddBlocks.enabled` - `spark.storage.decommission.shuffleBlocks.enabled` ### Why are the changes needed? This will help users use the storage decommissioning feature more easily. ### Does this PR introduce _any_ user-facing change? Yes, this will reduce the number of required configurations for decommission users. However, these two configurations are disabled by two configurations still. ### How was this patch tested? Pass the CIs.
This PR updates previous Manifest for KafkaInputDStream's Decoder to ClassTag, also fix the problem addressed in SPARK-2103.
Previous Java interface cannot actually get the type of Decoder, so when using this Manifest to reconstruct the decode object will meet reflection exception.
Also for other two Java interfaces, ClassTag[String] is useless because calling Scala API will get the right implicit ClassTag.
Current Kafka unit test cannot actually verify the interface. I've tested these interfaces in my local and distribute settings.