Re: Review Request 45457: Mitigate error-prone queries by disallowing no-op filters.
> On March 29, 2016, 4:20 p.m., John Sirois wrote: > > How would these queries produce incorrect results as compared to previous > > queries? Perviously, a check for the collection being null would have had > > to have been made by the caller (or a check for !IsSet) to skip the calls > > you've added precondition checks to. The IsSet calls are now gone, meaning > > the call sites using those have been semantically adjusted. I think this > > only leaves the callers who pass null of which there should be none due to > > the semantic change as well. > > Bill Farner wrote: > If i understand you correctly - the problem is with call sites that > assume `Query.slaveScoped(ImmutableList.of()` translates into `SELECT xyz > WHERE slave_host IN ()` (always returning 0 rows). > > However, the semantic is now such that the above query is equivalent to > `SELECT xyz WHERE 1`. > > Bill Farner wrote: > Oh, and to complete the thought - i am assuming that callers never mean > `WHERE 1` when they call `Query.slaveScoped(x)`. > > John Sirois wrote: > I'm assuming the opposite. A generic query builder using this tool might > work through criteria, some of which are unset. The unset criteria should > map to an always true ckause (WHERE 1), they should never map to an always > false clause (IN ()). I say this from 10k feet above this code though, not > taking real call sites in-mind. It may be there is no such code and so your > checks are fine in practice. > > Bill Farner wrote: > | It may be there is no such code and so your checks are fine in practice. > > The reverse may also be true :-) > > I honestly don't have a strong opinion on this. I'm okay if we just want > to recognize that the semantic has changed and we need to revisit call sites. > > Bill Farner wrote: > For some anecdotal evidence - i surveyed the call sites (bad on me, it > was much less effort than i anticipated), and did not find any other > potentially-vulnerable callers. So this patch may be much ado about nothing. > > Zameer - what do you think? > > Zameer Manji wrote: > I also feel this patch may be much ado about nothing. sgtm, discarding - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45457/#review125978 --- On March 29, 2016, 4:01 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45457/ > --- > > (Updated March 29, 2016, 4:01 p.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Bugs: AURORA-1652 > https://issues.apache.org/jira/browse/AURORA-1652 > > > Repository: aurora > > > Description > --- > > Short of surveying call sites, i don't know the impact of this change. > However, my sense is that it's still better to fail fast than produce > incorrect results (due to the recent change in semantics of filtering by an > empty iterable). > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/base/Query.java > c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac > > Diff: https://reviews.apache.org/r/45457/diff/ > > > Testing > --- > > > Thanks, > > Bill Farner > >
Re: Review Request 45457: Mitigate error-prone queries by disallowing no-op filters.
> On March 29, 2016, 4:20 p.m., John Sirois wrote: > > How would these queries produce incorrect results as compared to previous > > queries? Perviously, a check for the collection being null would have had > > to have been made by the caller (or a check for !IsSet) to skip the calls > > you've added precondition checks to. The IsSet calls are now gone, meaning > > the call sites using those have been semantically adjusted. I think this > > only leaves the callers who pass null of which there should be none due to > > the semantic change as well. > > Bill Farner wrote: > If i understand you correctly - the problem is with call sites that > assume `Query.slaveScoped(ImmutableList.of()` translates into `SELECT xyz > WHERE slave_host IN ()` (always returning 0 rows). > > However, the semantic is now such that the above query is equivalent to > `SELECT xyz WHERE 1`. > > Bill Farner wrote: > Oh, and to complete the thought - i am assuming that callers never mean > `WHERE 1` when they call `Query.slaveScoped(x)`. > > John Sirois wrote: > I'm assuming the opposite. A generic query builder using this tool might > work through criteria, some of which are unset. The unset criteria should > map to an always true ckause (WHERE 1), they should never map to an always > false clause (IN ()). I say this from 10k feet above this code though, not > taking real call sites in-mind. It may be there is no such code and so your > checks are fine in practice. > > Bill Farner wrote: > | It may be there is no such code and so your checks are fine in practice. > > The reverse may also be true :-) > > I honestly don't have a strong opinion on this. I'm okay if we just want > to recognize that the semantic has changed and we need to revisit call sites. > > Bill Farner wrote: > For some anecdotal evidence - i surveyed the call sites (bad on me, it > was much less effort than i anticipated), and did not find any other > potentially-vulnerable callers. So this patch may be much ado about nothing. > > Zameer - what do you think? I also feel this patch may be much ado about nothing. - Zameer --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45457/#review125978 --- On March 29, 2016, 4:01 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45457/ > --- > > (Updated March 29, 2016, 4:01 p.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Bugs: AURORA-1652 > https://issues.apache.org/jira/browse/AURORA-1652 > > > Repository: aurora > > > Description > --- > > Short of surveying call sites, i don't know the impact of this change. > However, my sense is that it's still better to fail fast than produce > incorrect results (due to the recent change in semantics of filtering by an > empty iterable). > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/base/Query.java > c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac > > Diff: https://reviews.apache.org/r/45457/diff/ > > > Testing > --- > > > Thanks, > > Bill Farner > >
Re: Review Request 45457: Mitigate error-prone queries by disallowing no-op filters.
> On March 29, 2016, 4:20 p.m., John Sirois wrote: > > How would these queries produce incorrect results as compared to previous > > queries? Perviously, a check for the collection being null would have had > > to have been made by the caller (or a check for !IsSet) to skip the calls > > you've added precondition checks to. The IsSet calls are now gone, meaning > > the call sites using those have been semantically adjusted. I think this > > only leaves the callers who pass null of which there should be none due to > > the semantic change as well. > > Bill Farner wrote: > If i understand you correctly - the problem is with call sites that > assume `Query.slaveScoped(ImmutableList.of()` translates into `SELECT xyz > WHERE slave_host IN ()` (always returning 0 rows). > > However, the semantic is now such that the above query is equivalent to > `SELECT xyz WHERE 1`. > > Bill Farner wrote: > Oh, and to complete the thought - i am assuming that callers never mean > `WHERE 1` when they call `Query.slaveScoped(x)`. > > John Sirois wrote: > I'm assuming the opposite. A generic query builder using this tool might > work through criteria, some of which are unset. The unset criteria should > map to an always true ckause (WHERE 1), they should never map to an always > false clause (IN ()). I say this from 10k feet above this code though, not > taking real call sites in-mind. It may be there is no such code and so your > checks are fine in practice. > > Bill Farner wrote: > | It may be there is no such code and so your checks are fine in practice. > > The reverse may also be true :-) > > I honestly don't have a strong opinion on this. I'm okay if we just want > to recognize that the semantic has changed and we need to revisit call sites. For some anecdotal evidence - i surveyed the call sites (bad on me, it was much less effort than i anticipated), and did not find any other potentially-vulnerable callers. So this patch may be much ado about nothing. Zameer - what do you think? - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45457/#review125978 --- On March 29, 2016, 4:01 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45457/ > --- > > (Updated March 29, 2016, 4:01 p.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Bugs: AURORA-1652 > https://issues.apache.org/jira/browse/AURORA-1652 > > > Repository: aurora > > > Description > --- > > Short of surveying call sites, i don't know the impact of this change. > However, my sense is that it's still better to fail fast than produce > incorrect results (due to the recent change in semantics of filtering by an > empty iterable). > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/base/Query.java > c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac > > Diff: https://reviews.apache.org/r/45457/diff/ > > > Testing > --- > > > Thanks, > > Bill Farner > >
Re: Review Request 45457: Mitigate error-prone queries by disallowing no-op filters.
> On March 29, 2016, 4:20 p.m., John Sirois wrote: > > How would these queries produce incorrect results as compared to previous > > queries? Perviously, a check for the collection being null would have had > > to have been made by the caller (or a check for !IsSet) to skip the calls > > you've added precondition checks to. The IsSet calls are now gone, meaning > > the call sites using those have been semantically adjusted. I think this > > only leaves the callers who pass null of which there should be none due to > > the semantic change as well. > > Bill Farner wrote: > If i understand you correctly - the problem is with call sites that > assume `Query.slaveScoped(ImmutableList.of()` translates into `SELECT xyz > WHERE slave_host IN ()` (always returning 0 rows). > > However, the semantic is now such that the above query is equivalent to > `SELECT xyz WHERE 1`. > > Bill Farner wrote: > Oh, and to complete the thought - i am assuming that callers never mean > `WHERE 1` when they call `Query.slaveScoped(x)`. > > John Sirois wrote: > I'm assuming the opposite. A generic query builder using this tool might > work through criteria, some of which are unset. The unset criteria should > map to an always true ckause (WHERE 1), they should never map to an always > false clause (IN ()). I say this from 10k feet above this code though, not > taking real call sites in-mind. It may be there is no such code and so your > checks are fine in practice. | It may be there is no such code and so your checks are fine in practice. The reverse may also be true :-) I honestly don't have a strong opinion on this. I'm okay if we just want to recognize that the semantic has changed and we need to revisit call sites. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45457/#review125978 --- On March 29, 2016, 4:01 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45457/ > --- > > (Updated March 29, 2016, 4:01 p.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Bugs: AURORA-1652 > https://issues.apache.org/jira/browse/AURORA-1652 > > > Repository: aurora > > > Description > --- > > Short of surveying call sites, i don't know the impact of this change. > However, my sense is that it's still better to fail fast than produce > incorrect results (due to the recent change in semantics of filtering by an > empty iterable). > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/base/Query.java > c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac > > Diff: https://reviews.apache.org/r/45457/diff/ > > > Testing > --- > > > Thanks, > > Bill Farner > >
Re: Review Request 45457: Mitigate error-prone queries by disallowing no-op filters.
> On March 29, 2016, 5:20 p.m., John Sirois wrote: > > How would these queries produce incorrect results as compared to previous > > queries? Perviously, a check for the collection being null would have had > > to have been made by the caller (or a check for !IsSet) to skip the calls > > you've added precondition checks to. The IsSet calls are now gone, meaning > > the call sites using those have been semantically adjusted. I think this > > only leaves the callers who pass null of which there should be none due to > > the semantic change as well. > > Bill Farner wrote: > If i understand you correctly - the problem is with call sites that > assume `Query.slaveScoped(ImmutableList.of()` translates into `SELECT xyz > WHERE slave_host IN ()` (always returning 0 rows). > > However, the semantic is now such that the above query is equivalent to > `SELECT xyz WHERE 1`. > > Bill Farner wrote: > Oh, and to complete the thought - i am assuming that callers never mean > `WHERE 1` when they call `Query.slaveScoped(x)`. I'm assuming the opposite. A generic query builder using this tool might work through criteria, some of which are unset. The unset criteria should map to an always true ckause (WHERE 1), they should never map to an always false clause (IN ()). I say this from 10k feet above this code though, not taking real call sites in-mind. It may be there is no such code and so your checks are fine in practice. - John --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45457/#review125978 --- On March 29, 2016, 5:01 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45457/ > --- > > (Updated March 29, 2016, 5:01 p.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Bugs: AURORA-1652 > https://issues.apache.org/jira/browse/AURORA-1652 > > > Repository: aurora > > > Description > --- > > Short of surveying call sites, i don't know the impact of this change. > However, my sense is that it's still better to fail fast than produce > incorrect results (due to the recent change in semantics of filtering by an > empty iterable). > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/base/Query.java > c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac > > Diff: https://reviews.apache.org/r/45457/diff/ > > > Testing > --- > > > Thanks, > > Bill Farner > >
Re: Review Request 45457: Mitigate error-prone queries by disallowing no-op filters.
> On March 29, 2016, 4:20 p.m., John Sirois wrote: > > How would these queries produce incorrect results as compared to previous > > queries? Perviously, a check for the collection being null would have had > > to have been made by the caller (or a check for !IsSet) to skip the calls > > you've added precondition checks to. The IsSet calls are now gone, meaning > > the call sites using those have been semantically adjusted. I think this > > only leaves the callers who pass null of which there should be none due to > > the semantic change as well. > > Bill Farner wrote: > If i understand you correctly - the problem is with call sites that > assume `Query.slaveScoped(ImmutableList.of()` translates into `SELECT xyz > WHERE slave_host IN ()` (always returning 0 rows). > > However, the semantic is now such that the above query is equivalent to > `SELECT xyz WHERE 1`. Oh, and to complete the thought - i am assuming that callers never mean `WHERE 1` when they call `Query.slaveScoped(x)`. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45457/#review125978 --- On March 29, 2016, 4:01 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45457/ > --- > > (Updated March 29, 2016, 4:01 p.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Bugs: AURORA-1652 > https://issues.apache.org/jira/browse/AURORA-1652 > > > Repository: aurora > > > Description > --- > > Short of surveying call sites, i don't know the impact of this change. > However, my sense is that it's still better to fail fast than produce > incorrect results (due to the recent change in semantics of filtering by an > empty iterable). > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/base/Query.java > c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac > > Diff: https://reviews.apache.org/r/45457/diff/ > > > Testing > --- > > > Thanks, > > Bill Farner > >
Re: Review Request 45457: Mitigate error-prone queries by disallowing no-op filters.
> On March 29, 2016, 4:20 p.m., John Sirois wrote: > > How would these queries produce incorrect results as compared to previous > > queries? Perviously, a check for the collection being null would have had > > to have been made by the caller (or a check for !IsSet) to skip the calls > > you've added precondition checks to. The IsSet calls are now gone, meaning > > the call sites using those have been semantically adjusted. I think this > > only leaves the callers who pass null of which there should be none due to > > the semantic change as well. If i understand you correctly - the problem is with call sites that assume `Query.slaveScoped(ImmutableList.of()` translates into `SELECT xyz WHERE slave_host IN ()` (always returning 0 rows). However, the semantic is now such that the above query is equivalent to `SELECT xyz WHERE 1`. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45457/#review125978 --- On March 29, 2016, 4:01 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45457/ > --- > > (Updated March 29, 2016, 4:01 p.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Bugs: AURORA-1652 > https://issues.apache.org/jira/browse/AURORA-1652 > > > Repository: aurora > > > Description > --- > > Short of surveying call sites, i don't know the impact of this change. > However, my sense is that it's still better to fail fast than produce > incorrect results (due to the recent change in semantics of filtering by an > empty iterable). > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/base/Query.java > c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac > > Diff: https://reviews.apache.org/r/45457/diff/ > > > Testing > --- > > > Thanks, > > Bill Farner > >
Re: Review Request 45457: Mitigate error-prone queries by disallowing no-op filters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45457/#review125978 --- How would these queries produce incorrect results as compared to previous queries? Perviously, a check for the collection being null would have had to have been made by the caller (or a check for !IsSet) to skip the calls you've added precondition checks to. The IsSet calls are now gone, meaning the call sites using those have been semantically adjusted. I think this only leaves the callers who pass null of which there should be none due to the semantic change as well. - John Sirois On March 29, 2016, 5:01 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45457/ > --- > > (Updated March 29, 2016, 5:01 p.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Bugs: AURORA-1652 > https://issues.apache.org/jira/browse/AURORA-1652 > > > Repository: aurora > > > Description > --- > > Short of surveying call sites, i don't know the impact of this change. > However, my sense is that it's still better to fail fast than produce > incorrect results (due to the recent change in semantics of filtering by an > empty iterable). > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/base/Query.java > c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac > > Diff: https://reviews.apache.org/r/45457/diff/ > > > Testing > --- > > > Thanks, > > Bill Farner > >
Re: Review Request 45457: Mitigate error-prone queries by disallowing no-op filters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45457/#review125972 --- Master (ec29ac1) is green with this patch. ./build-support/jenkins/build.sh However, it appears that it might lack test coverage. I will refresh this build result if you post a review containing "@ReviewBot retry" - Aurora ReviewBot On March 29, 2016, 11:01 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45457/ > --- > > (Updated March 29, 2016, 11:01 p.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Bugs: AURORA-1652 > https://issues.apache.org/jira/browse/AURORA-1652 > > > Repository: aurora > > > Description > --- > > Short of surveying call sites, i don't know the impact of this change. > However, my sense is that it's still better to fail fast than produce > incorrect results (due to the recent change in semantics of filtering by an > empty iterable). > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/base/Query.java > c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac > > Diff: https://reviews.apache.org/r/45457/diff/ > > > Testing > --- > > > Thanks, > > Bill Farner > >
