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

Hotfix/arrebol retry #85

Closed
wants to merge 6 commits into from
Closed

Hotfix/arrebol retry #85

wants to merge 6 commits into from

Conversation

wesleymonte
Copy link
Contributor

@wesleymonte wesleymonte commented Feb 24, 2020

#81
When Saps tries to get an Arrebol Job, and that Job does not exist, it keeps trying to get it, even if that job never exists again. Leading to thread locking.

@@ -20,11 +21,13 @@

public List<JobSubmitted> returnAllJobsSubmitted();
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove public terms from the methods of the Arrebol interface

selectedTasks = selectTasks();
submitTasks(selectedTasks);
} catch (Exception e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a message next to the trace, something like "Error while schedule tasks"

addTimestampTaskInCatalog(task, "updates task [" + task.getTaskId() + "] timestamp");
addTimestampTaskInCatalog(task, "updates task [" + task.getTaskId() + "] timestamp");
} catch (Exception e) {
updateStateInCatalog(task, ImageTaskState.FAILED, SapsImage.AVAILABLE, SapsImage.NON_EXISTENT_DATA,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that this was a joint decision with @thiagomanel with you, maybe another option is to ignore it and move on to the next one (because nothing was changed in the task information in the Catalog), but log it.

@@ -383,43 +397,44 @@ private void checker() {
String jobId = job.getJobId();
SapsImage task = job.getImageTask();

JobResponseDTO jobResponse = getJobByIdInArrebol(jobId, "gets job by ID [" + jobId + "]");
LOGGER.debug("Job [" + jobId + "] information returned from Arrebol: " + jobResponse);
if (!wasJobFound(jobResponse)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the dead method wasJobFound(...)

@@ -20,11 +21,13 @@

public List<JobSubmitted> returnAllJobsSubmitted();

public JobResponseDTO checkStatusJobById(String jobId) throws GetJobException;
public JobResponseDTO checkStatusJobById(String jobId)
throws GetJobException, ArrebolConnectException;

public List<JobResponseDTO> checkStatusJobByName(String JobName) throws GetJobException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a fixme reporting that the method is waiting for Arrebol to implement this feature


public List<JobResponseDTO> checkStatusJobByName(String JobName) throws GetJobException;

public String checkStatusJobString(String jobId) throws GetJobException;
public String checkStatusJobString(String jobId) throws GetJobException, ArrebolConnectException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently this method is dead, check if it is still used in any case, if you do not remove it

@@ -63,12 +65,13 @@ public JobResponseDTO checkStatusJobById(String jobId) throws GetJobException {
}

@Override
public String checkStatusJobString(String jobId) throws GetJobException {
public String checkStatusJobString(String jobId) throws GetJobException, ArrebolConnectException {
Copy link
Contributor

Choose a reason for hiding this comment

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

As already stated in the Arrebol interface, apparently this method is dead, check if it is still used in any case, if you do not remove it


public class ArrebolConnectException extends Exception {

public ArrebolConnectException(String message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove, it is not used

@@ -15,7 +15,7 @@ public SubmitJobRetry(Arrebol arrebol, SapsJob job) {
}

@Override
public String run() throws Exception, SubmitJobException {
public String run() throws Exception {
return arrebol.addJob(job);
Copy link
Contributor

Choose a reason for hiding this comment

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

Change Exception for the other exceptions that are thrown in this addJob method, let's make this clear in the method signature. If I'm not mistaken they are: SubmitJobException, UnsupportedEncodingException, ArrebolConnectException

@thiagoyeds
Copy link
Contributor

This repo does not contain code

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

Successfully merging this pull request may close these issues.

Incorrect actions in the Arrebol service retry approach (GetJobException)
2 participants