Skip to content
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

ExececutionService could be simplified and documented #10

Open
askask opened this issue Jun 2, 2017 · 5 comments
Open

ExececutionService could be simplified and documented #10

askask opened this issue Jun 2, 2017 · 5 comments
Assignees
Labels
Milestone

Comments

@askask
Copy link
Contributor

askask commented Jun 2, 2017

It has several similar (and apparently unused?) methods, maybe some of them could be removed.

For some methods it's not obvious how to implement them, so it would be nice if there was some documentation.

@fkaercher fkaercher added the docs label Jun 2, 2017
@dankwart-de
Copy link
Contributor

Could you say which methods you mean? I already simplified things, just give me a hint :)

@askask
Copy link
Contributor Author

askask commented Jun 6, 2017

  • handleServiceBasedJobExitStatus I don't know what this method is supposed to do? Apparently it still works if I implement it as an empty method, so is it needed?

  • ExecutionResult execute(String command, boolean waitForIncompatibleClassChangeError, OutputStream outputStream) and ExecutionResult execute(Command command, boolean waitFor) are unused

  • Is it necessary to have execute methods with both command as String and Command; and both with and without waitFor? Those could also be implemented as default methods if required

@dankwart-de
Copy link
Contributor

I did not know how to rename it properly. The method is used to let any software using BE set the job exit stuff like e.g. the job result.

They are not unused in Roddy. We can investigate, if we can reduce the number of methods.

@vinjana
Copy link
Contributor

vinjana commented Oct 4, 2017

Maybe we can move all existing execution service classes into RoddyToolLib? (SSHJ, jsch, local)

@askask
Copy link
Contributor Author

askask commented Oct 17, 2017

@vinjana I think that might be a good idea.

I also think it's strange that there is a method File queryWorkingDirectory(). First, the value is only used to set the log directory, not he working directory. Second, wouldn't it make more sense to set both the default log directory and default working directory in the job manager?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

4 participants