Re: Review Request: Wengine Task Querier Thread: OODT-310
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4961/ --- (Updated 2012-05-27 21:58:01.392196) Review request for oodt, brian Foster, Ricky Nguyen, Paul Ramirez, Sheryl John, and Thomas Bennett. Changes --- Updated unit test, and changed the locking/synchronization logic. All unit tests pass and I think this is good to go. I'll commit it tonight or tomorrow. Thanks all for the review! Summary --- Task Querier thread for OODT-310. See javadocs on: https://builds.apache.org/job/oodt-trunk/javadoc/org/apache/oodt/cas/workflow/engine/TaskQuerier.html This addresses bug OODT-310. https://issues.apache.org/jira/browse/OODT-310 Diffs (updated) - ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/ParallelProcessor.java 1342993 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/SequentialProcessor.java 1342993 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskProcessor.java 1342993 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskQuerier.java 1342993 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/WorkflowProcessor.java 1342993 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/WorkflowProcessorBuilder.java PRE-CREATION ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java PRE-CREATION Diff: https://reviews.apache.org/r/4961/diff Testing --- Includes unit test, that currently isn't passing. I think I know why (something up with my threading logic and synchronized keywords) but wanted to throw it up for review. I'll likely be working on this tomorrow or the following evening. Thanks, Chris
Re: Review Request: Wengine Task Querier Thread: OODT-310
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4961/ --- (Updated 2012-05-27 05:35:54.936237) Review request for oodt, brian Foster, Ricky Nguyen, Paul Ramirez, Sheryl John, and Thomas Bennett. Changes --- Updated with suggestions from bfoster, and sherylj. - created WorkflowProcessorBuilder class - updated and fixed race condition in QuerierThread (now return a copy) - updated condition checking (e.g., using isEmpty rather than ==0) - adding check for isNull (assert) in unit test - added not done processor All unit tests now pass. I'd like to commit this in the next 24 hours. Summary --- Task Querier thread for OODT-310. See javadocs on: https://builds.apache.org/job/oodt-trunk/javadoc/org/apache/oodt/cas/workflow/engine/TaskQuerier.html This addresses bug OODT-310. https://issues.apache.org/jira/browse/OODT-310 Diffs (updated) - ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/ParallelProcessor.java 1342993 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/SequentialProcessor.java 1342993 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskProcessor.java 1342993 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskQuerier.java 1342993 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/WorkflowProcessor.java 1342993 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/WorkflowProcessorBuilder.java PRE-CREATION ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java PRE-CREATION Diff: https://reviews.apache.org/r/4961/diff Testing --- Includes unit test, that currently isn't passing. I think I know why (something up with my threading logic and synchronized keywords) but wanted to throw it up for review. I'll likely be working on this tomorrow or the following evening. Thanks, Chris
Re: Review Request: Wengine Task Querier Thread: OODT-310
On 2012-05-02 19:20:13, brian Foster wrote:
./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java,
line 79
https://reviews.apache.org/r/4961/diff/1/?file=105932#file105932line79
When Success/done is passed in, the processor created is incorrect
since it has a sub-processor in Queued/waiting
Chris Mattmann wrote:
Thoughts on how to fix?
Sheryl John wrote:
If 'anydoneStates'/done is passed, then don't add taskProcessor and
return the 'done' processor?
Chris Mattmann wrote:
Good idea, Sheryl, I will try that.
Maybe change getProcessor signature to something along the lines of:
private WorkflowProcessor getProcessor(String id,
double priority,
String stateName,
String categoryName,
ListWorkflowProcessor subProcessors);
Then just make some helper methods which build test WorkflowProcessors... Maybe
even make a WorkflowProcessorBuilder class:
public class WorkflowProcessorBuilder {
private String id;
private double priority;
private ListWorkflowProcessor subProcessors;
private WorkflowProcessorBuilder() {
subProcessors = Lists.newArrayList();
}
public static WorkflowProcessorBuilder aWorkflowProcessorBuilder() {
return new WorkflowProcessorBuilder();
}
public WorkflowProcessorBuilder withId(String id) {
this.id = id;
return this;
}
public WorkflowProcessorBuilder withPriority(double priority) {
this.priority = priority;
return this;
}
public WorkflowProcessorBuilder with(WorkflowProcessorBuilder wpb) {
subProcessors.add(wpb.build());
return this;
}
...
...
...
public WorkflowProcessor build() {
WorkflowProcessor wp = new ...
wp.setId(id);
wp.setPriority(priority);
wp.setSubProcessors(subProcessors);
...
return wp;
}
}
- brian
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4961/#review7482
---
On 2012-05-02 05:08:45, Chris Mattmann wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4961/
---
(Updated 2012-05-02 05:08:45)
Review request for oodt, brian Foster, Ricky Nguyen, Paul Ramirez, Sheryl
John, and Thomas Bennett.
Summary
---
Task Querier thread for OODT-310. See javadocs on:
https://builds.apache.org/job/oodt-trunk/javadoc/org/apache/oodt/cas/workflow/engine/TaskQuerier.html
This addresses bug OODT-310.
https://issues.apache.org/jira/browse/OODT-310
Diffs
-
./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskQuerier.java
1332505
./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/WorkflowProcessor.java
1331866
./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java
PRE-CREATION
Diff: https://reviews.apache.org/r/4961/diff
Testing
---
Includes unit test, that currently isn't passing. I think I know why
(something up with my threading logic and synchronized keywords) but wanted
to throw it up for review. I'll likely be working on this tomorrow or the
following evening.
Thanks,
Chris
Re: Review Request: Wengine Task Querier Thread: OODT-310
On 2012-05-02 19:20:13, brian Foster wrote:
./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java,
line 79
https://reviews.apache.org/r/4961/diff/1/?file=105932#file105932line79
When Success/done is passed in, the processor created is incorrect
since it has a sub-processor in Queued/waiting
Chris Mattmann wrote:
Thoughts on how to fix?
Sheryl John wrote:
If 'anydoneStates'/done is passed, then don't add taskProcessor and
return the 'done' processor?
Chris Mattmann wrote:
Good idea, Sheryl, I will try that.
brian Foster wrote:
Maybe change getProcessor signature to something along the lines of:
private WorkflowProcessor getProcessor(String id,
double priority,
String stateName,
String categoryName,
ListWorkflowProcessor
subProcessors);
Then just make some helper methods which build test WorkflowProcessors...
Maybe even make a WorkflowProcessorBuilder class:
public class WorkflowProcessorBuilder {
private String id;
private double priority;
private ListWorkflowProcessor subProcessors;
private WorkflowProcessorBuilder() {
subProcessors = Lists.newArrayList();
}
public static WorkflowProcessorBuilder aWorkflowProcessorBuilder() {
return new WorkflowProcessorBuilder();
}
public WorkflowProcessorBuilder withId(String id) {
this.id = id;
return this;
}
public WorkflowProcessorBuilder withPriority(double priority) {
this.priority = priority;
return this;
}
public WorkflowProcessorBuilder with(WorkflowProcessorBuilder wpb) {
subProcessors.add(wpb.build());
return this;
}
...
...
...
public WorkflowProcessor build() {
WorkflowProcessor wp = new ...
wp.setId(id);
wp.setPriority(priority);
wp.setSubProcessors(subProcessors);
...
return wp;
}
}
actually:
public static WorkflowProcessorBuilder aWorkflowProcessorBuilder() {
return new WorkflowProcessorBuilder();
}
should be:
public static WorkflowProcessorBuilder aWorkflowProcessor() {
return new WorkflowProcessorBuilder();
}
- brian
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4961/#review7482
---
On 2012-05-02 05:08:45, Chris Mattmann wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4961/
---
(Updated 2012-05-02 05:08:45)
Review request for oodt, brian Foster, Ricky Nguyen, Paul Ramirez, Sheryl
John, and Thomas Bennett.
Summary
---
Task Querier thread for OODT-310. See javadocs on:
https://builds.apache.org/job/oodt-trunk/javadoc/org/apache/oodt/cas/workflow/engine/TaskQuerier.html
This addresses bug OODT-310.
https://issues.apache.org/jira/browse/OODT-310
Diffs
-
./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskQuerier.java
1332505
./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/WorkflowProcessor.java
1331866
./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java
PRE-CREATION
Diff: https://reviews.apache.org/r/4961/diff
Testing
---
Includes unit test, that currently isn't passing. I think I know why
(something up with my threading logic and synchronized keywords) but wanted
to throw it up for review. I'll likely be working on this tomorrow or the
following evening.
Thanks,
Chris
Re: Review Request: Wengine Task Querier Thread: OODT-310
On 2012-05-02 19:20:13, brian Foster wrote: ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java, line 79 https://reviews.apache.org/r/4961/diff/1/?file=105932#file105932line79 When Success/done is passed in, the processor created is incorrect since it has a sub-processor in Queued/waiting Chris Mattmann wrote: Thoughts on how to fix? Sheryl John wrote: If 'anydoneStates'/done is passed, then don't add taskProcessor and return the 'done' processor? Good idea, Sheryl, I will try that. - Chris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4961/#review7482 --- On 2012-05-02 05:08:45, Chris Mattmann wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4961/ --- (Updated 2012-05-02 05:08:45) Review request for oodt, brian Foster, Ricky Nguyen, Paul Ramirez, Sheryl John, and Thomas Bennett. Summary --- Task Querier thread for OODT-310. See javadocs on: https://builds.apache.org/job/oodt-trunk/javadoc/org/apache/oodt/cas/workflow/engine/TaskQuerier.html This addresses bug OODT-310. https://issues.apache.org/jira/browse/OODT-310 Diffs - ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskQuerier.java 1332505 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/WorkflowProcessor.java 1331866 ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java PRE-CREATION Diff: https://reviews.apache.org/r/4961/diff Testing --- Includes unit test, that currently isn't passing. I think I know why (something up with my threading logic and synchronized keywords) but wanted to throw it up for review. I'll likely be working on this tomorrow or the following evening. Thanks, Chris
Re: Review Request: Wengine Task Querier Thread: OODT-310
On 2012-05-02 19:20:13, brian Foster wrote: ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java, line 79 https://reviews.apache.org/r/4961/diff/1/?file=105932#file105932line79 When Success/done is passed in, the processor created is incorrect since it has a sub-processor in Queued/waiting Chris Mattmann wrote: Thoughts on how to fix? If 'anydoneStates'/done is passed, then don't add taskProcessor and return the 'done' processor? - Sheryl --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4961/#review7482 --- On 2012-05-02 05:08:45, Chris Mattmann wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4961/ --- (Updated 2012-05-02 05:08:45) Review request for oodt, brian Foster, Ricky Nguyen, Paul Ramirez, Sheryl John, and Thomas Bennett. Summary --- Task Querier thread for OODT-310. See javadocs on: https://builds.apache.org/job/oodt-trunk/javadoc/org/apache/oodt/cas/workflow/engine/TaskQuerier.html This addresses bug OODT-310. https://issues.apache.org/jira/browse/OODT-310 Diffs - ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskQuerier.java 1332505 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/WorkflowProcessor.java 1331866 ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java PRE-CREATION Diff: https://reviews.apache.org/r/4961/diff Testing --- Includes unit test, that currently isn't passing. I think I know why (something up with my threading logic and synchronized keywords) but wanted to throw it up for review. I'll likely be working on this tomorrow or the following evening. Thanks, Chris
Re: Review Request: Wengine Task Querier Thread: OODT-310
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4961/#review7482 --- like the patch!... few comments ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskQuerier.java https://reviews.apache.org/r/4961/#comment16558 Why do you want Holding category to be returned? ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskQuerier.java https://reviews.apache.org/r/4961/#comment16556 This should return a copy of runnableProcessors here... probably where your synchronization issue is ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java https://reviews.apache.org/r/4961/#comment16554 getRunnableProcessors() is never null... probably should be a unit-test to insure that functionality ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java https://reviews.apache.org/r/4961/#comment16555 !querier.getRunnableProcessors().isEmpty() ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java https://reviews.apache.org/r/4961/#comment16565 When Success/done is passed in, the processor created is incorrect since it has a sub-processor in Queued/waiting ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java https://reviews.apache.org/r/4961/#comment16560 Should probably add a NOT done processor here - brian On 2012-05-02 05:08:45, Chris Mattmann wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4961/ --- (Updated 2012-05-02 05:08:45) Review request for oodt, brian Foster, Ricky Nguyen, Paul Ramirez, Sheryl John, and Thomas Bennett. Summary --- Task Querier thread for OODT-310. See javadocs on: https://builds.apache.org/job/oodt-trunk/javadoc/org/apache/oodt/cas/workflow/engine/TaskQuerier.html This addresses bug OODT-310. https://issues.apache.org/jira/browse/OODT-310 Diffs - ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/TaskQuerier.java 1332505 ./trunk/workflow/src/main/java/org/apache/oodt/cas/workflow/engine/WorkflowProcessor.java 1331866 ./trunk/workflow/src/test/org/apache/oodt/cas/workflow/engine/TestTaskQuerier.java PRE-CREATION Diff: https://reviews.apache.org/r/4961/diff Testing --- Includes unit test, that currently isn't passing. I think I know why (something up with my threading logic and synchronized keywords) but wanted to throw it up for review. I'll likely be working on this tomorrow or the following evening. Thanks, Chris
