thinkharderdev commented on PR #560: URL: https://github.com/apache/arrow-ballista/pull/560#issuecomment-1398473523
> I'm not as familiar with the overall Ballista architecture as I'd like to be, but this PR still makes a lot of sense. > > I _love_ seeing this stuff come out of the `executor_manager` which seemed like it didn't have a clear boundary on what it's role was and go into a more self-contained `cluster.rs`. > > I like that it encapsulates all the HA session management stuff, because I think we'll need options like Redis and others in different enterprise environments. > > The only thing that makes me sad is seeing 2 values (state + backend) get passed around everywhere instead of 1. That's really not the fault of this PR however, as mentioned in my other comment, I think we need to clean the plumbing up around config in general. Yeah, I agree. I'm workring on the second part of this PR (which will implement the `JobState` interface) and will take a crack at cleaning this up. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
