-
Notifications
You must be signed in to change notification settings - Fork 28k
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
[SPARK-5961][Streaming]Allow specific nodes in a Spark Streaming cluster to be preferred as Receiver Worker Nodes #5114
Conversation
…ferred as Receiver Worker Nodes versus regular Spark Worker Nodes
Test build #28941 has finished for PR 5114 at commit
|
@@ -107,9 +107,6 @@ abstract class Receiver[T](val storageLevel: StorageLevel) extends Serializable | |||
*/ | |||
def onStop() | |||
|
|||
/** Override this to specify a preferred location (hostname). */ | |||
def preferredLocation : Option[String] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why move this function from Receiver
to ReceiverInputDStream
? I think for people who customized Receiver
and override this function may fail to compile. Is there any specific reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jerryshao thanks for your review, I have not thinked about people who customized Receiver and override this function, I just want to provide a function to set preferred location for the ReceiverInputDStream/Receiver already exsits in Spark Streaming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Receiver
is an interface for user, I think it would be better not remove this method, one possible way you could is to pass the argument through ReceiverInputDStream
to this buy adding some methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried, but didn't find a good way to pass the argument through ReceiverInputDStream
to this by adding some methods.
IMO, ReceiverInputDStream
is also an interface for user, user can overide preferredLocation (ReceiverInputDStream
) in their own implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with this change, custom receiver implementations will need to re-implement this code? I am not sure that is a good idea, since this is part of a stable interface. For one, that breaks custom code and the other aspect is that it sets a bad precedent.
Test build #28975 has finished for PR 5114 at commit
|
storageLevel: StorageLevel, | ||
enableDecompression: Boolean | ||
) extends ReceiverInputDStream[SparkFlumeEvent](ssc_) { | ||
@transient ssc_ : StreamingContext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What has changed in these lines? Why are they in the diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for codestyle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like changes to 4 space indent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is the correct indentation now.
Test build #31048 has started for PR 5114 at commit
|
Test build #35157 has finished for PR 5114 at commit
|
@jerryshao @ArcherShao any updates on this? @harishreedharan |
I personally suggest not changing the interface of |
@ArcherShao Any comments on other's comments? If you are unable to work on this PR, mind closing it? |
Upgrades Iceberg to 0.13.0.5-apple * Internal: Change the no bloom filter check to be getBloomFilterOffset… (https://github.pie.apple.com/IPR/apache-incubator-iceberg/commit/f3f156bcf4670e0f0d5da3749ff1ff87482c40a5) … * Core: Remove Usage of Sets.Union in Loop (apache#5114) (https://github.pie.apple.com/IPR/apache-incubator-iceberg/commit/440ba4d679c66b7b44896c5cfa22c2a29ef437bb) Co-authored-by: Russell Spitzer <russell.spitzer@gmail.com>
1.The function ’setPreferredLocation‘ is in class ReceiverInputDStream.
2.FlumeInputDStream take data send node as the defalut PreferredLocation