Re: Review Request 18526: Add support for slaveHosts set in TaskQuery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/#review35836 --- src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java https://reviews.apache.org/r/18526/#comment66606 I believe this entire line can be replaced with: return jobKey.asSet(); src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java https://reviews.apache.org/r/18526/#comment66621 This is O(n) for n keys in the map. More ideal is to iterate over the keys. I don't know of a 'multi-get' for Map/Multmap, but i'd support use of that if it does. Also, not new, but it's a good idea to wrap this with Iterables.unmodifiableIterable(). - Bill Farner On Feb. 28, 2014, 1:18 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- (Updated Feb. 28, 2014, 1:18 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/python/apache/aurora/client/api/sla.py 131c357d60fd00740b51055f555d56c599124d15 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
Re: Review Request 18526: Add support for slaveHosts set in TaskQuery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/#review35861 --- src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java https://reviews.apache.org/r/18526/#comment66647 That would create an empty SetIJobKey in case jobkey is missing whereas I want OptionalSetIJobKey. This code will be refactored shortly when I get to AURORA-235. src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java https://reviews.apache.org/r/18526/#comment66654 Unfortunately, I could not find anything that could be converted into getAll() on a multimap. The filter was the best I could find. Yes, it does iterate over keys to apply filtering predicate as evident from the sources. I put .filter against a looped get() for a multimap size of ~500k (10k keys, up to 100 values per key) and, as expected, the perf of a looped get() degraded quickly with the increasing query key-set size: Query set size Perf 1-10 .filter 50% slower 100 .filter 20% slower 1000 .filter 250% faster However, a looped get() outperformed .filter for anything below 100 keys. Since the majority of our queries are well under 10 keys using a looped get() seems to be a better alternative. Converting to it now. - Maxim Khutornenko On Feb. 28, 2014, 1:18 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- (Updated Feb. 28, 2014, 1:18 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/python/apache/aurora/client/api/sla.py 131c357d60fd00740b51055f555d56c599124d15 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
Re: Review Request 18526: Add support for slaveHosts set in TaskQuery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- (Updated Feb. 28, 2014, 10:53 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- CR comments. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs (updated) - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/python/apache/aurora/client/api/sla.py 131c357d60fd00740b51055f555d56c599124d15 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
Re: Review Request 18526: Add support for slaveHosts set in TaskQuery.
On Feb. 28, 2014, 10:53 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java, line 80 https://reviews.apache.org/r/18526/diff/3/?file=506631#file506631line80 That would create an empty SetIJobKey in case jobkey is missing whereas I want OptionalSetIJobKey. This code will be refactored shortly when I get to AURORA-235. Ah, right - thanks. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/#review35861 --- On Feb. 28, 2014, 10:53 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- (Updated Feb. 28, 2014, 10:53 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/python/apache/aurora/client/api/sla.py 131c357d60fd00740b51055f555d56c599124d15 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
Re: Review Request 18526: Add support for slaveHosts set in TaskQuery.
On Feb. 27, 2014, 1:38 a.m., Kevin Sweeney wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 364 https://reviews.apache.org/r/18526/diff/1/?file=504721#file504721line364 Mind adding a ticket number (filing one if it doesn't exist)? Added ticket tracking removal. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/#review35589 --- On Feb. 26, 2014, 6:17 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- (Updated Feb. 26, 2014, 6:17 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
Re: Review Request 18526: Add support for slaveHosts set in TaskQuery.
On Feb. 27, 2014, 11:19 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java, line 76 https://reviews.apache.org/r/18526/diff/2/?file=506086#file506086line76 I know why you're doing this here, but mind keeping it out for now? Ignore this comment, i intended to delete it after i saw how the related code changed. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/#review35722 --- On Feb. 27, 2014, 4:42 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- (Updated Feb. 27, 2014, 4:42 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
Re: Review Request 18526: Add support for slaveHosts set in TaskQuery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/#review35723 --- src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java https://reviews.apache.org/r/18526/#comment66445 Good point, completely missed the optional meaning there. Question: do we treat an empty value set in a TaskQuery as (1) or (2)? I am leaning towards (1) as a less strict and more intuitive. Any objections? - Maxim Khutornenko On Feb. 27, 2014, 4:42 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- (Updated Feb. 27, 2014, 4:42 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
Re: Review Request 18526: Add support for slaveHosts set in TaskQuery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/#review35729 --- src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java https://reviews.apache.org/r/18526/#comment66450 On a second thought, it's easier to reason about all this if we treat set and non-set values the same way, namely non-null empty sets would return empty results. - Maxim Khutornenko On Feb. 27, 2014, 4:42 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- (Updated Feb. 27, 2014, 4:42 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
Re: Review Request 18526: Add support for slaveHosts set in TaskQuery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/#review35738 --- src/main/java/org/apache/aurora/scheduler/base/Query.java https://reviews.apache.org/r/18526/#comment66459 Great suggestion. Done. src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java https://reviews.apache.org/r/18526/#comment66460 Ignored. src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java https://reviews.apache.org/r/18526/#comment66461 I realized there were no users of the TaskQuery.slaveHost anywhere. Dropping this field from thrift. src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java https://reviews.apache.org/r/18526/#comment66462 n/a src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/18526/#comment66463 Dropped. - Maxim Khutornenko On Feb. 27, 2014, 4:42 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- (Updated Feb. 27, 2014, 4:42 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
Re: Review Request 18526: Add support for slaveHosts set in TaskQuery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- (Updated Feb. 28, 2014, 1:18 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- CR comments. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs (updated) - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/python/apache/aurora/client/api/sla.py 131c357d60fd00740b51055f555d56c599124d15 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
Review Request 18526: Add support for slaveHosts set in TaskQuery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18526/ --- Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-232 https://issues.apache.org/jira/browse/AURORA-232 Repository: aurora Description --- The slaveHosts set-based field will eventually replace a string field. All internal queries are converted to use the new field. The old one is still supported for queries coming from the web (until client side is refactored). Diffs - src/main/java/org/apache/aurora/scheduler/base/Query.java b9f207c740362fafff0257988f52b6179025d58d src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 7337044bacd052c516e4d78a2993946d472ef91c src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java d1ab503e4edac86afcb8884a074a87b7536de3f7 src/main/thrift/org/apache/aurora/gen/api.thrift cd60f47bf34b4a634004e2ad9eadad37aa1556bb src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 884f589a2cbea918ecbfedf457f42d7cc9254c95 src/test/resources/org/apache/aurora/gen/api.thrift.md5 fafb5100443482e662db453429c5259f2ab80ae5 Diff: https://reviews.apache.org/r/18526/diff/ Testing --- ./build-support/jenkins/build.sh Thanks, Maxim Khutornenko
