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

Add IRemoteJobManager interface #398

Merged
merged 8 commits into from Mar 26, 2015
Merged

Conversation

FedeMPouzols
Copy link
Contributor

This addresses ticket #11123 and it is an intermediate step in a chain of tickets towards generic remote algorithms. This just adds an interface. It will become more testable when the RemoteJobManager factory and the specialized job managers are added in follow-up PRs.

To test: this PR just adds an interface. See the ticket for details on the "remote job manager" design. Check that it is well documented and that the documentation reads well. As this goes in principle in Framework/Kernel, it would be great if an experienced Mantid developer double checks that everything is fine and follows the rules/conventions/style.

@FedeMPouzols FedeMPouzols added this to the Release 3.4 milestone Mar 16, 2015
@FedeMPouzols FedeMPouzols added the Framework Issues and pull requests related to components in the Framework label Mar 16, 2015
@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

1 similar comment
@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

1 similar comment
@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@FedeMPouzols FedeMPouzols changed the title Add IRemoteJobManger interface Add IRemoteJobManager interface Mar 18, 2015
* properly.
* @throws std::runtime_error If authentication fails
*/
virtual void authenticate(std::string &username, std::string &password) = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a good reason that the parameters are non-const references when all other methods in the interface accept a const reference to a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ups, I'll fix it, there's definitely no reason.

@martyngigg
Copy link
Member

Most of our interface classes are in API, is there a reason this is in Kernel?

@FedeMPouzols
Copy link
Contributor Author

I guess that I just put it in the same place where the current RemoteJobManager class lives.
But yes, it doesn't make much sense to me, and actually the RemoteJobManagerFactory that comes in a follow-up ticket is inside API. So I'll move this interface into API.

@martyngigg
Copy link
Member

Kernel/API are a somewhat arbitrary split so I just wondered on the reason. Thanks

@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

1 similar comment
@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@FedeMPouzols
Copy link
Contributor Author

Seemingly unrelated and random system test failue.. Jenkins, retest this please

@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

1 similar comment
@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@FedeMPouzols
Copy link
Contributor Author

Jenkins could you retest this please?

@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

4 similar comments
@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@mantid-roman mantid-roman self-assigned this Mar 24, 2015
std::string startDate;
/// Date-time the job finished. No particular format can be
/// assumed
std::string completionTime;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the time-related have a more appropriate type (DateAndType for example)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'm going to change the type of those three to DateAndTime.
The schedulers that we have in practice (LSF and Mantid API/MOAB) use different formats but both can be converted to DateAndTime.

@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

3 similar comments
@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@FedeMPouzols
Copy link
Contributor Author

Jenkins, retest this please

@FedeMPouzols
Copy link
Contributor Author

Jenkins... retest this please

mantid-roman added a commit that referenced this pull request Mar 26, 2015
@mantid-roman mantid-roman merged commit 581d861 into master Mar 26, 2015
@mantid-roman mantid-roman deleted the 11123_add_IRemoteJobManager branch March 26, 2015 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues and pull requests related to components in the Framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants