Skip to content
This repository has been archived by the owner on Sep 15, 2021. It is now read-only.

Provide a better error for client-side shard filtering. #23

Merged
merged 3 commits into from
Mar 21, 2015
Merged

Provide a better error for client-side shard filtering. #23

merged 3 commits into from
Mar 21, 2015

Conversation

deflaux
Copy link
Contributor

@deflaux deflaux commented Mar 13, 2015

Now that we are doing client-side filtering for strict shard boundaries, we need to check that we are requesting the field that the filter will require and return a useful error if not.

Also increased the default number of bases per shard. This yields a reasonable number of tasks for sharded jobs running over the whole human genome.

This yields a reasonable number of tasks for sharded jobs running over the whole genome.
Now that we are doing client-side filtering for strict shard boundaries, we need to check that we are requesting the field that the filter will require and return a useful error if not.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 54.96% when pulling a5f96c2 on deflaux:master into ee2f74a on googlegenomics:master.

@pgrosu
Copy link

pgrosu commented Mar 13, 2015

Nicole, I think it's ready to merge. I want to give it a try through Dataflow.

Thanks,
Paul

@deflaux
Copy link
Contributor Author

deflaux commented Mar 13, 2015

@calbach ptal when you have time? And let me know if you have an alternate suggestion for this check.

@pgrosu
Copy link

pgrosu commented Mar 13, 2015

Nicole, it's good enough. Just mention it in the Readme that you are being restrictive to generate uniqueness in the shards. Until we find a better solution - which would require some more deep thinking - let's just release it. Just provide something reasonable as a reason so users understand why.

Paul

@Override
public void initialize(GenomicsRequest<?> search) {
if (null != fields) {
for (String requiredField : REQUIRED_STRICT_SHARD_FIELDS.keySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this could be extracted to a common utility which accepts the fields string and the map of patterns to be matched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored!

@coveralls
Copy link

Coverage Status

Coverage increased (+1.18%) to 56.18% when pulling 00c633e on deflaux:master into ee2f74a on googlegenomics:master.

calbach added a commit that referenced this pull request Mar 21, 2015
Provide a better error for client-side shard filtering.
@calbach calbach merged commit 517f581 into googlegenomics:master Mar 21, 2015
@pgrosu
Copy link

pgrosu commented Mar 21, 2015

Fantabulous - I love it! Thanks guys!

Have a great weekend!
~p

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

Successfully merging this pull request may close these issues.

None yet

4 participants