-
Notifications
You must be signed in to change notification settings - Fork 198
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
Fix Scheduler UI in Docker image #251
Conversation
@yahoNanJing @thinkharderdev @avantgardnerio This is ready for review. Once this is merged, I will create a PR to add a list of jobs and the ability to view job status. |
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
@@ -85,7 +85,7 @@ fn with_data_server<T: AsLogicalPlan + Clone, U: 'static + AsExecutionPlan>( | |||
pub fn get_routes<T: AsLogicalPlan + Clone, U: 'static + AsExecutionPlan>( | |||
scheduler_server: SchedulerServer<T, U>, | |||
) -> BoxedFilter<(impl Reply,)> { | |||
let routes = warp::path("state") | |||
let routes = warp::path!("api" / "state") |
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.
overloading the /
operator? Nothing to do with this PR, but not a fan after seeing abuse in C++ & C#.
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.
Agreed. I am just beginning to learn about the warp framework, so will see if this can be avoided as I learn more.
Thanks @andygrove. LGTM. Verified at my env. |
Thanks for the reviews @yahoNanJing and @avantgardnerio |
Which issue does this PR close?
Closes #250
Rationale for this change
The UI now shows information!
What changes are included in this PR?
Try and proxy API requests to the scheduler process
Are there any user-facing changes?
nginx config