Enrich ApiJobStore to support filtering #23
Conversation
venisa
force-pushed
the
Enrich-the-ApiJobStore-interface
branch
from
eba614c to
1099084
Compare
|
@archolewa @cdeszaq @dayamr Please review |
venisa
force-pushed
the
Enrich-the-ApiJobStore-interface
branch
2 times, most recently
from
74e935d to
60ac75a
Compare
| * This method takes a Set of ApiJobStoreFilters, ANDS them by default, and returns a cold observable that emits a | ||
| * stream of JobRows which satisfy the given filter. | ||
| * <p> | ||
| * Every field may not be filterable for every implementation of the `ApiJobStore` as the efficiency of filtering is |
There was a problem hiding this comment.
Should replace the markdown style backticks with the JavaDoc {@code ...}. I too wish that JavaDocs supported Markdown, but alas this is not Elixir (also I think Java predates Markdown).
|
Finished my first pass. |
venisa
force-pushed
the
Enrich-the-ApiJobStore-interface
branch
from
3e9bd60 to
77c7dbb
Compare
venisa
force-pushed
the
Enrich-the-ApiJobStore-interface
branch
from
77c7dbb to
5a62c89
Compare
|
@archolewa Addressed all comments |
| } | ||
|
|
||
| /** | ||
| * This method checks if the given JobRow satisfies the given JobRowFilter and returnd true if it does. |
There was a problem hiding this comment.
Typo. returnd should be returns.
| * This method takes a Set of JobRowFilters, ANDS them by default, and returns a cold observable that emits a | ||
| * stream of JobRows which satisfy the given filter. | ||
| * <p> | ||
| * Every field may not be filterable for every implementation of the {@code ApiJobStore} as the efficiency of |
There was a problem hiding this comment.
'Any of the fields may be not' would be slightly clearer than 'Every field may not be'. As written it seems to mean that sometimes all the fields are not filterable and sometimes they are all filterable.
|
I'm happy with this. 👍 Once the issue with the mobstor reference is cleaned up. |
| * filtering is dependent on the backing store. An IllegalArgumentException is thrown if filtering on any given | ||
| * field is not supported. | ||
| * | ||
| * @param jobRowFilters A List of JobRowFilters where each JobRowFilter contains the JobField to be |
There was a problem hiding this comment.
Note: The type is Set but this calls it a List. Which should it be?
|
|
||
| @Override | ||
| public Observable<JobRow> getFilteredRows(Set<JobRowFilter> jobRowFilter) | ||
| throws IllegalArgumentException { |
There was a problem hiding this comment.
Can unwrap.
Also, we should probably document what this will return, since the NoOp version is a little odd, in general, and it may not be clear what filtering on the NoOp store would do.
venisa
force-pushed
the
Enrich-the-ApiJobStore-interface
branch
from
e2e6688 to
6623363
Compare
archolewa
force-pushed
the
Enrich-the-ApiJobStore-interface
branch
from
0bc0bd7 to
ec2c7d0
Compare
ApiJobStore now supports filtering JobRows
Added filtering logic to HashJobStore
Added
ApiJobStoreFilterwhich stores filter information like the JobField to be inspected, the values to be compared and the filter operation to be performed.