-
Notifications
You must be signed in to change notification settings - Fork 331
Update worker contract for Spark-3.0. #311
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
| { | ||
| System.IO.Stream inputStream = serverSocket.InputStream; | ||
| System.IO.Stream outputStream = serverSocket.OutputStream; | ||
| using (ISocketWrapper serverSocket = serverListener.Accept()) |
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.
did you want to use the new C#8 usings here as well ?
using ISocketWrapper serverSocket = serverListener.Accept();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.
There is a scope here (notice that there is an Assert after this block. It may be OK, but don't want to explore this in this PR (the other changes are suggested by the IDE).
| /// </summary> | ||
| internal class TaskContext | ||
| { | ||
| internal class Resource |
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.
Are we following any special rules in terms of the ordering of elements within a C# file ? Something like https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1201.md ?
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 moved this to the bottom, but we need to have style cop in our repo. I will create an issue and follow up.
| (Value == other.Value) && | ||
| Addresses.SequenceEqual(Addresses); | ||
| } | ||
| public override int GetHashCode() |
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.
| public override int GetHashCode() | |
| public override int GetHashCode() |
suhsteve
left a comment
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.
LGTM
This PR updates contract b/w JVM and CLR to process simple UDF.
All the UDFs under Basic.cs work.