-
Notifications
You must be signed in to change notification settings - Fork 202
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
[Part 10]: Task Executor Starter and other related classes #160
Conversation
TaskExecutorStarter is the starting point for the task executor from the worker side. Initializing this class and starting it as part of the runtime framework (guice/springboot) would start the task executor thread capable of running any task from the client side.
api "com.typesafe.akka:akka-http-jackson_$scalaBinaryVersion:$akkaHttpVersion" | ||
api "com.typesafe.akka:akka-http-caching_$scalaBinaryVersion:$akkaHttpVersion" | ||
api "com.typesafe.akka:akka-stream_$scalaBinaryVersion:$akkaVersion" | ||
compile group: 'com.typesafe.akka', name: "akka-stream_$scalaBinaryVersion", version: "$akkaVersion" |
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.
is this line the same as the one above?
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.
you are right. let me clean this up.
}); | ||
} | ||
|
||
public static org.apache.hadoop.fs.FileSystem create(URI fsUri) throws IOException { |
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.
it's quite confusing that this returns a hadoop filesystem. consider renaming this class to FileSystemUtils
or something else.
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.
+1
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.
done
return this; | ||
} | ||
|
||
public RpcSystem getRpcSystem() { |
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.
you may want to make these getXXX()
methods private.
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.
good suggestion. done
} | ||
|
||
public static RpcSystem load(Configuration configuration) { | ||
return new MantisAkkaRpcSystemLoader().loadRpcSystem(configuration); |
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.
umm, would it better to have this as a singleton?
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.
yes it does. made the change.
return factory.create(fsUri); | ||
} else { | ||
throw new IllegalArgumentException( | ||
String.format("Unknown schema", fsUri.getScheme().toString())); |
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.
nit: pl add a %s
here for the scheme.
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.
thanks for spotting this. done.
}); | ||
} | ||
|
||
public static org.apache.hadoop.fs.FileSystem create(URI fsUri) throws IOException { |
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.
+1
@@ -17,34 +17,34 @@ | |||
apply plugin: 'application' | |||
|
|||
ext { | |||
akkaVersion = '2.5.23' | |||
akkaHttpVersion = '10.1.8' | |||
akkaVersion = '2.6.15' |
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.
The newer ones are 2.6.18 and 10.2.9. Do you think we can advance to those ones instead (asking since I have those version upgrade in some of my code already).
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.
These are the versions that the flink RPC library brings in. I want the versions to match that.
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 see. Let me use these ones on my change too then.
TaskExecutorStarter is the starting point for the task executor from the worker side. Initializing this class and starting it as part of the runtime framework (guice/springboot) would start the task executor thread capable of running any task from the client side.
Context
This is the last missing piece to start the task executor from the runtime framework.
Checklist
./gradlew build
compiles code correctly./gradlew test
passes all testsCONTRIBUTING.md