Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-11-28 Thread Joshua Cohen 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review157136
---


Ship it!




Ship It!

- Joshua Cohen


On Nov. 28, 2016, 8:34 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Nov. 28, 2016, 8:34 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1762
> https://issues.apache.org/jira/browse/AURORA-1762
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 84265066001dee79df6bc16de6de9a165e912b9b 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> b521620badff76e8fab0bdf31f8a73c4019b2121 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> 77187bc19fb1f783d1b820d324c7fe18e509365f 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/TestUtils.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-11-28 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review157132
---


Ship it!




Master (d56f8c6) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Nov. 28, 2016, 8:34 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Nov. 28, 2016, 8:34 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1762
> https://issues.apache.org/jira/browse/AURORA-1762
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 84265066001dee79df6bc16de6de9a165e912b9b 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> b521620badff76e8fab0bdf31f8a73c4019b2121 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> 77187bc19fb1f783d1b820d324c7fe18e509365f 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/TestUtils.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-11-28 Thread Pradyumna Kaushik 


> On Nov. 28, 2016, 4:03 p.m., Joshua Cohen wrote:
> > Sorry for the delay on this, everything looks good to me, just a few 
> > style/maintainability nits, then it's good to go!

@jcohen. I have made the necessary changes and have posted the patch for 
review. Thank you.


- Pradyumna


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review157037
---


On Nov. 28, 2016, 8:34 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Nov. 28, 2016, 8:34 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1762
> https://issues.apache.org/jira/browse/AURORA-1762
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 84265066001dee79df6bc16de6de9a165e912b9b 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> b521620badff76e8fab0bdf31f8a73c4019b2121 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> 77187bc19fb1f783d1b820d324c7fe18e509365f 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/TestUtils.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-11-23 Thread Stephan Erb 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review156728
---


Ship it!




Looks good to me! @jcohen, what do you think?

Sorry about the delay. Looks like we forgot about that review.

- Stephan Erb


On Nov. 3, 2016, 11:46 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Nov. 3, 2016, 11:46 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1762
> https://issues.apache.org/jira/browse/AURORA-1762
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 84265066001dee79df6bc16de6de9a165e912b9b 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> b521620badff76e8fab0bdf31f8a73c4019b2121 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> 77187bc19fb1f783d1b820d324c7fe18e509365f 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/TestUtils.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-11-08 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review155378
---


Ship it!




Master (793711f) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Nov. 3, 2016, 10:46 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Nov. 3, 2016, 10:46 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1762
> https://issues.apache.org/jira/browse/AURORA-1762
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 84265066001dee79df6bc16de6de9a165e912b9b 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> b521620badff76e8fab0bdf31f8a73c4019b2121 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> 77187bc19fb1f783d1b820d324c7fe18e509365f 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/TestUtils.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-11-08 Thread Stephan Erb 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review155373
---



@ReviewBot retry

- Stephan Erb


On Nov. 3, 2016, 11:46 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Nov. 3, 2016, 11:46 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1762
> https://issues.apache.org/jira/browse/AURORA-1762
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 84265066001dee79df6bc16de6de9a165e912b9b 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> b521620badff76e8fab0bdf31f8a73c4019b2121 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> 77187bc19fb1f783d1b820d324c7fe18e509365f 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/TestUtils.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-11-08 Thread Pradyumna Kaushik 


> On Nov. 4, 2016, 8:21 a.m., Aurora ReviewBot wrote:
> > Master (171a8f7) is red with this patch.
> >   ./build-support/jenkins/build.sh
> > 
> > > Test coverage missing for org/apache/aurora/scheduler/http/Offers
> >   Test coverage missing for org/apache/aurora/scheduler/http/PendingTasks
> >   Test coverage missing for org/apache/aurora/scheduler/app/VolumeParser
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/configuration/executor/ExecutorModule
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/pruning/TaskHistoryPruner$1
> >   Test coverage missing for org/apache/aurora/scheduler/base/Jobs
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/stats/ResourceCounter$1
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/stats/ResourceCounter
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/stats/ResourceCounter$Metric
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/http/api/security/ModuleParser
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/log/mesos/MesosLog$LogStream
> >   Test coverage missing for org/apache/aurora/scheduler/log/mesos/MesosLog
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/log/mesos/MesosLog$LogStream$OpStats
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/log/mesos/MesosLog$LogStream$1
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/log/mesos/MesosLog$LogStream$LogEntry
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/log/mesos/MesosLog$LogStream$LogPosition
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/storage/db/PruneVictim
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/state/TaskAssigner$TaskAssignerImpl
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/http/api/GsonMessageBodyHandler
> >   Test coverage missing for org/apache/aurora/scheduler/http/api/ApiBeta
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/http/api/GsonMessageBodyHandler$1
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/cron/quartz/AuroraCronJob
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/scheduling/RescheduleCalculator$RescheduleCalculatorImpl$1
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/mesos/TaskStatusStats$3
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/mesos/TaskStatusStats$2
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/mesos/TaskStatusStats$1
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/mesos/TaskStatusStats
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter$PreemptionVictimFilterImpl
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter$PreemptionVictimFilterImpl$2
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter$PreemptionVictimFilterImpl$1
> >   Test coverage missing for 
> > org/apache/aurora/scheduler/storage/backup/StorageBackup$StorageBackupImpl$BackupConfig
> >   Test coverage missing for org/apache/aurora/scheduler/HostOffer$1
> > 
> > * Try:
> > Run with --stacktrace option to get the stack trace. Run with --info or 
> > --debug option to get more log output.
> > ==
> > 
> > BUILD FAILED
> > 
> > Total time: 4 mins 37.823 secs
> > 
> > 
> > I will refresh this build result if you post a review containing 
> > "@ReviewBot retry"

@ReviewBot retry


- Pradyumna


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review154877
---


On Nov. 3, 2016, 10:46 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Nov. 3, 2016, 10:46 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1762
> https://issues.apache.org/jira/browse/AURORA-1762
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 84265066001dee79df6bc16de6de9a165e912b9b 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> b521620badff76e8fab0bdf31f8a73c4019b2121 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> 77187bc19fb1f783d1b820d324c7fe18e509365f 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-11-03 Thread Pradyumna Kaushik 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/
---

(Updated Nov. 3, 2016, 10:46 p.m.)


Review request for Aurora and Joshua Cohen.


Changes
---

Dropped @VisibleForTesting annotation for public constructor of 
NearestFit.java. Modified NearestFit#getPendingReasons(...) to use streams 
instead. Reverted OffersTest.java. Added another TaskGroup to 
PendingTasksTest#testOffers() to ensure that the reasons are mapped to the 
correct TaskGroupKey. Compared objects instead of strings in 
PendingTasksTest.java and NearestFitTest#getPendingReasons(...). Removed 
unnecessary tabs in the source code. Moved PendingTasksTest#makeTask(...) and 
NearestFitTest#makeTask(...) to a utility class 
src/test/java/org/apache/aurora/scheduler/http/TestUtils.java. Used 
JsonNode#asText() instead of JsonNode#toString()#replace(...) in 
PendingTasks#getOffers().


Bugs: AURORA-1762
https://issues.apache.org/jira/browse/AURORA-1762


Repository: aurora


Description
---

Added the 'reason' to the /pendingTasks endpoint


Diffs (updated)
-

  config/legacy_untested_classes.txt 84265066001dee79df6bc16de6de9a165e912b9b 
  src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
  src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
f783e7ff220573915524a1efc27141193d19fa6c 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
b521620badff76e8fab0bdf31f8a73c4019b2121 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
77187bc19fb1f783d1b820d324c7fe18e509365f 
  src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
9e3573252cf37153180b1fc5ab9150bab0299c99 
  src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/http/TestUtils.java PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
d9b3cc672f42c50b2a2a142733d26c0725bbc864 

Diff: https://reviews.apache.org/r/51993/diff/


Testing
---

./build-support/jenkins/build.sh
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh


Thanks,

Pradyumna Kaushik

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-27 Thread Santhosh Kumar Shanmugham 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review154033
---




src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java (line 73)


Use `pendingTask.get("name").asText()` instead?


- Santhosh Kumar Shanmugham


On Oct. 26, 2016, 6:01 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 26, 2016, 6:01 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1762
> https://issues.apache.org/jira/browse/AURORA-1762
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt ee4d3d7e537356ae99ba3a90a3631749067662f3 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-27 Thread Santhosh Kumar Shanmugham 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review153979
---




src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (line 62)


Drop the annotation, since it is used in actual source code.



src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (lines 136 - 
151)


This can be changed like so and avoid the joining-splitting voodoo,

  public synchronized Map getPendingReasons(
  Iterable taskGroups) {
return StreamSupport.stream(taskGroups.spliterator(), false).map(t -> {
  List reasons = getNearestFit(t.getKey()).stream()
  .map(Veto::getReason).collect(Collectors.toList());
  return new HashMap.SimpleEntry<>(t.getKey().toString(), reasons);
}).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
  }



src/test/java/org/apache/aurora/scheduler/http/OffersTest.java (line 17)


Revert this file.



src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java (line 59)


We will need more than 1 `TaskGroup` here to ensure that the reasons are 
mapped to the correct `TaskGroupKey`.



src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java (line 84)


Assert using an object instead of `toString`?



src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java (line 50)


fix - tab



src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java (line 52)


fix - tab



src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java (line 54)


fix - tab



src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java (line 56)


fix - tab



src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java (line 96)


fix - tab



src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java (lines 
99 - 125)


This method is already in PendingTasksTest, it will be better to 
encapsulate this into a common TestUtils class and reuse in both places.



src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java (lines 
140 - 141)


fix - tab



src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java (lines 
165 - 166)


Use direct object comparison instead of `toString()`, see 
`assertNearest(...)` for example.


- Santhosh Kumar Shanmugham


On Oct. 26, 2016, 6:01 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 26, 2016, 6:01 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1762
> https://issues.apache.org/jira/browse/AURORA-1762
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt ee4d3d7e537356ae99ba3a90a3631749067662f3 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-26 Thread Pradyumna Kaushik 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/
---

(Updated Oct. 26, 2016, 6:01 p.m.)


Review request for Aurora and Joshua Cohen.


Bugs: AURORA-1762
https://issues.apache.org/jira/browse/AURORA-1762


Repository: aurora


Description
---

Added the 'reason' to the /pendingTasks endpoint


Diffs
-

  config/legacy_untested_classes.txt ee4d3d7e537356ae99ba3a90a3631749067662f3 
  src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
  src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
f783e7ff220573915524a1efc27141193d19fa6c 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
5d319557057e27fd5fc6d3e553e9ca9139399c50 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
d390c07522d22e43d79ce4370985f3643ef021ca 
  src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
9e3573252cf37153180b1fc5ab9150bab0299c99 
  src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
d9b3cc672f42c50b2a2a142733d26c0725bbc864 

Diff: https://reviews.apache.org/r/51993/diff/


Testing
---

./build-support/jenkins/build.sh
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh


Thanks,

Pradyumna Kaushik

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-26 Thread Zameer Manji 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review153968
---



Pradyumna,

Can you rebase and refresh this review?

Joshua, do you still have bandwidth to review this?

- Zameer Manji


On Oct. 17, 2016, 4:35 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 17, 2016, 4:35 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt ee4d3d7e537356ae99ba3a90a3631749067662f3 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-17 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review152992
---



This patch does not apply cleanly against master (ac8b802), do you need to 
rebase?

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 17, 2016, 11:35 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 17, 2016, 11:35 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt ee4d3d7e537356ae99ba3a90a3631749067662f3 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-17 Thread Pradyumna Kaushik 


> On Oct. 17, 2016, 11:45 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java, line 144
> > 
> >
> > This split looks kind of magic. What does it do?
> 
> Pradyumna Kaushik wrote:
> Joiner.on(...) would return a comma separated string of reasons. I need 
> to convert that to a List. I am splitting the comma separated string 
> (returned by Joiner.on(...)) with  comma  number of spaces after> so that I end up with a String[] of reasons. I pass 
> this String[] of reasons to Arrays.asList(...) to generate a List.
> 
> I will comment that line of code so that it is understandable as to what 
> I am doing.
> 
> Pradyumna Kaushik wrote:
> I can just make NearestFit#getPendingReasons() return Map String[]> instead of Map. This way, I won't need 
> to convert String[] into List using Arrays.asList(...). Please let me 
> know which would be better.

Just posted the changes mentioned above for review.


- Pradyumna


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review152852
---


On Oct. 17, 2016, 11:35 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 17, 2016, 11:35 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt ee4d3d7e537356ae99ba3a90a3631749067662f3 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-17 Thread Pradyumna Kaushik 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/
---

(Updated Oct. 17, 2016, 11:35 p.m.)


Review request for Aurora and Joshua Cohen.


Repository: aurora


Description
---

Added the 'reason' to the /pendingTasks endpoint


Diffs (updated)
-

  config/legacy_untested_classes.txt ee4d3d7e537356ae99ba3a90a3631749067662f3 
  src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
  src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
f783e7ff220573915524a1efc27141193d19fa6c 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
5d319557057e27fd5fc6d3e553e9ca9139399c50 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
d390c07522d22e43d79ce4370985f3643ef021ca 
  src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
9e3573252cf37153180b1fc5ab9150bab0299c99 
  src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
d9b3cc672f42c50b2a2a142733d26c0725bbc864 

Diff: https://reviews.apache.org/r/51993/diff/


Testing
---

./build-support/jenkins/build.sh
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh


Thanks,

Pradyumna Kaushik

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-17 Thread Pradyumna Kaushik 


> On Oct. 17, 2016, 11:45 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java, line 144
> > 
> >
> > This split looks kind of magic. What does it do?
> 
> Pradyumna Kaushik wrote:
> Joiner.on(...) would return a comma separated string of reasons. I need 
> to convert that to a List. I am splitting the comma separated string 
> (returned by Joiner.on(...)) with  comma  number of spaces after> so that I end up with a String[] of reasons. I pass 
> this String[] of reasons to Arrays.asList(...) to generate a List.
> 
> I will comment that line of code so that it is understandable as to what 
> I am doing.

I can just make NearestFit#getPendingReasons() return Map instead of Map. This way, I won't need to 
convert String[] into List using Arrays.asList(...). Please let me know 
which would be better.


- Pradyumna


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review152852
---


On Oct. 15, 2016, 6:56 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 15, 2016, 6:56 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt ee4d3d7e537356ae99ba3a90a3631749067662f3 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-17 Thread Pradyumna Kaushik 


> On Oct. 17, 2016, 11:45 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java, line 144
> > 
> >
> > This split looks kind of magic. What does it do?

Joiner.on(...) would return a comma separated string of reasons. I need to 
convert that to a List. I am splitting the comma separated string 
(returned by Joiner.on(...)) with  comma  so that I end up with a String[] of reasons. I pass 
this String[] of reasons to Arrays.asList(...) to generate a List.

I will comment that line of code so that it is understandable as to what I am 
doing.


> On Oct. 17, 2016, 11:45 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java, lines 
> > 67-70
> > 
> >
> > I am not sure if I understand this code correctly. How does this ensure 
> > we attatch the `reason` to the correct taskgroup in the json.

In NearestFit#getPendingReasons(...) I am using a LinkedHashMap<> to maintain 
insertion order and filling it in the order of items in taskGroups.getGroups().
So, the order in which the reasons are filled in, is the same as the order of 
items in taskGroups.getGroups().
This is why I can be sure that the reason would correspond to the correct 
pendingtask.

I can make a small change, where I am picking the value corresponding to the 
right key. In fact it would be much more clearer that way.


- Pradyumna


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review152852
---


On Oct. 15, 2016, 6:56 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 15, 2016, 6:56 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt ee4d3d7e537356ae99ba3a90a3631749067662f3 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-17 Thread Stephan Erb 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review152852
---




src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java (lines 67 - 70)


I am not sure if I understand this code correctly. How does this ensure we 
attatch the `reason` to the correct taskgroup in the json.



src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (line 144)


This split looks kind of magic. What does it do?


- Stephan Erb


On Oct. 15, 2016, 8:56 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 15, 2016, 8:56 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt ee4d3d7e537356ae99ba3a90a3631749067662f3 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 
> 9e3573252cf37153180b1fc5ab9150bab0299c99 
>   src/test/java/org/apache/aurora/scheduler/http/PendingTasksTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
> d9b3cc672f42c50b2a2a142733d26c0725bbc864 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-06 Thread Zameer Manji 
Just make them public and add a `@VisibleForTesting` annotation.

We do this in many places to make the code testable.

On Thu, Oct 6, 2016 at 2:55 PM, Pradyumna Kaushik 
wrote:

> Correction,
>
> I need to create mocks of TaskGroups.java and TaskGroup.java and not
> PendingTasks.java.
>
> On Thu, Oct 6, 2016 at 5:54 PM, Pradyumna Kaushik  > wrote:
>
>> Hi,
>>
>> I was writing the test file for PendingTasks.java and have a question.
>> The constructors for TaskGroups.java and TaskGroup.java are package scoped
>> and hence, are not accessible in test/java/org/apache/aurora/scheduler/http.
>> I need to be able to create mocks of pending tasks and hence, I don't think
>> I can use createMock(...) for the same. I currently have thought of 2 ways
>> to get around this.
>> 1. Make the constructors of TaskGroups.java and TaskGroup.java public.
>> 2. Use reflection and create instances, like shown below,
>> Constructor taskGroupsConstructor =
>> (Constructor) TaskGroups.class.getConstructors()[0];
>> taskGroupsConstructor.setAccessible(true);
>> taskGroupsConstructor.newInstance(...);
>>
>> Please let me know which method would be appropriate.
>>
>> On Wed, Oct 5, 2016 at 3:50 PM, Pradyumna Kaushik <
>> pkaus...@binghamton.edu> wrote:
>>
>>> Thank you. This clarifies things for me. I will get back to you in case
>>> anything pops up.
>>>
>>> On Wed, Oct 5, 2016 at 3:47 PM, Joshua Cohen  wrote:
>>>
 Well, I guess to be specific, the perfect place would be:
 src/test/java/org/apache/aurora/scheduler/http ;).

 On Wed, Oct 5, 2016 at 2:46 PM, Joshua Cohen  wrote:

> That would be the perfect place for the test, thanks for adding it!
>
> On Wed, Oct 5, 2016 at 2:43 PM, Pradyumna Kaushik <
> pkaus...@binghamton.edu> wrote:
>
>> Zameer,
>>
>> Alright. I will start making changes to incorporate Stephan's idea.
>> Also, I didn't seem to find an existing test for PendingTasks. Please
>> let me know whether I can go ahead and create PendingTasksTest.java in
>> src/test/java/scheduler/http. If not, then please let me know where I can
>> plug in the JUnit tests for /pendingTasks api.
>>
>> On Wed, Oct 5, 2016 at 3:21 PM, Zameer Manji 
>> wrote:
>>
>>> If you don't think it's too much work, I think Stephan's idea is
>>> ideal.
>>>
>>> The mutability of `reason` can result in bugs later.
>>>
>>> On Wed, Oct 5, 2016 at 11:31 AM, Pradyumna Kaushik <
>>> pkaus...@binghamton.edu> wrote:
>>>
 Hi,

 Should I go ahead and work on Stephan's idea of returning a
 Map  or should I hold it for now until the idea is a
 little more clear?

 On Tue, Oct 4, 2016 at 5:18 PM, Zameer Manji 
 wrote:

> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
>
> On October 4th, 2016, 2:10 p.m. PDT, *Stephan Erb* wrote:
>
> This change will be backwards incompatible. For example, it will 
> break the Prometheus Aurora exporter: 
> https://github.com/tommyulfsparre/aurora_exporter/blob/master/main.go#L101
>
> So the question: Do we consider this endpoint to be part of our 
> public API? If yes, how can we remain backwards compatible?
>
> I'm not sure if we do make this endpoint apart of our public API, but 
> I think we can make a backwards compatible change here.
>
> The current schema of the endpoint is list of objects where each 
> object has the keys:
> - penaltyMs (string)
> - taskIds (list of strings)
> - name (string)
>
> Surely we can add "reason" there as a list of strings.
>
>
> - Zameer
>
> On October 4th, 2016, 11:25 a.m. PDT, Pradyumna Kaushik wrote:
> Review request for Aurora and Joshua Cohen.
> By Pradyumna Kaushik.
>
> *Updated Oct. 4, 2016, 11:25 a.m.*
> *Repository: * aurora
> Description
>
> Added the 'reason' to the /pendingTasks endpoint
>
> Testing
>
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
> Diffs
>
>- src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java
>(c80e0c8adf80e12082a6952ae79b7d9cc960c5b6)
>- 
> src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java
>(f783e7ff220573915524a1efc27141193d19fa6c)
>- 
> src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java
>(5d319557057e27fd5fc6d3e553e9ca9139399c50)
>
> View Diff

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-05 Thread Joshua Cohen 
Well, I guess to be specific, the perfect place would be:
src/test/java/org/apache/aurora/scheduler/http ;).

On Wed, Oct 5, 2016 at 2:46 PM, Joshua Cohen  wrote:

> That would be the perfect place for the test, thanks for adding it!
>
> On Wed, Oct 5, 2016 at 2:43 PM, Pradyumna Kaushik  > wrote:
>
>> Zameer,
>>
>> Alright. I will start making changes to incorporate Stephan's idea.
>> Also, I didn't seem to find an existing test for PendingTasks. Please let
>> me know whether I can go ahead and create PendingTasksTest.java in
>> src/test/java/scheduler/http. If not, then please let me know where I can
>> plug in the JUnit tests for /pendingTasks api.
>>
>> On Wed, Oct 5, 2016 at 3:21 PM, Zameer Manji  wrote:
>>
>>> If you don't think it's too much work, I think Stephan's idea is ideal.
>>>
>>> The mutability of `reason` can result in bugs later.
>>>
>>> On Wed, Oct 5, 2016 at 11:31 AM, Pradyumna Kaushik <
>>> pkaus...@binghamton.edu> wrote:
>>>
 Hi,

 Should I go ahead and work on Stephan's idea of returning a
 Map  or should I hold it for now until the idea is a
 little more clear?

 On Tue, Oct 4, 2016 at 5:18 PM, Zameer Manji  wrote:

> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
>
> On October 4th, 2016, 2:10 p.m. PDT, *Stephan Erb* wrote:
>
> This change will be backwards incompatible. For example, it will break 
> the Prometheus Aurora exporter: 
> https://github.com/tommyulfsparre/aurora_exporter/blob/master/main.go#L101
>
> So the question: Do we consider this endpoint to be part of our public 
> API? If yes, how can we remain backwards compatible?
>
> I'm not sure if we do make this endpoint apart of our public API, but I 
> think we can make a backwards compatible change here.
>
> The current schema of the endpoint is list of objects where each object 
> has the keys:
> - penaltyMs (string)
> - taskIds (list of strings)
> - name (string)
>
> Surely we can add "reason" there as a list of strings.
>
>
> - Zameer
>
> On October 4th, 2016, 11:25 a.m. PDT, Pradyumna Kaushik wrote:
> Review request for Aurora and Joshua Cohen.
> By Pradyumna Kaushik.
>
> *Updated Oct. 4, 2016, 11:25 a.m.*
> *Repository: * aurora
> Description
>
> Added the 'reason' to the /pendingTasks endpoint
>
> Testing
>
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
> Diffs
>
>- src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java
>(c80e0c8adf80e12082a6952ae79b7d9cc960c5b6)
>- src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java
>(f783e7ff220573915524a1efc27141193d19fa6c)
>- src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java
>(5d319557057e27fd5fc6d3e553e9ca9139399c50)
>
> View Diff 
>



 --
 Thanks and Regards
 Pradyumna Kaushik

 *Graduate Student*
 *Department of Computer Science*
 *Thomas J. Watson School of Engineering and Applied Science*
 *Binghamton University*

>>>
>>>
>>>
>>> --
>>> Zameer Manji
>>>
>>
>>
>>
>> --
>> Thanks and Regards
>> Pradyumna Kaushik
>>
>> *Graduate Student*
>> *Department of Computer Science*
>> *Thomas J. Watson School of Engineering and Applied Science*
>> *Binghamton University*
>>
>
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-05 Thread Joshua Cohen 
That would be the perfect place for the test, thanks for adding it!

On Wed, Oct 5, 2016 at 2:43 PM, Pradyumna Kaushik 
wrote:

> Zameer,
>
> Alright. I will start making changes to incorporate Stephan's idea.
> Also, I didn't seem to find an existing test for PendingTasks. Please let
> me know whether I can go ahead and create PendingTasksTest.java in
> src/test/java/scheduler/http. If not, then please let me know where I can
> plug in the JUnit tests for /pendingTasks api.
>
> On Wed, Oct 5, 2016 at 3:21 PM, Zameer Manji  wrote:
>
>> If you don't think it's too much work, I think Stephan's idea is ideal.
>>
>> The mutability of `reason` can result in bugs later.
>>
>> On Wed, Oct 5, 2016 at 11:31 AM, Pradyumna Kaushik <
>> pkaus...@binghamton.edu> wrote:
>>
>>> Hi,
>>>
>>> Should I go ahead and work on Stephan's idea of returning a Map>> String>  or should I hold it for now until the idea is a little more clear?
>>>
>>> On Tue, Oct 4, 2016 at 5:18 PM, Zameer Manji  wrote:
>>>
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/51993/

 On October 4th, 2016, 2:10 p.m. PDT, *Stephan Erb* wrote:

 This change will be backwards incompatible. For example, it will break the 
 Prometheus Aurora exporter: 
 https://github.com/tommyulfsparre/aurora_exporter/blob/master/main.go#L101

 So the question: Do we consider this endpoint to be part of our public 
 API? If yes, how can we remain backwards compatible?

 I'm not sure if we do make this endpoint apart of our public API, but I 
 think we can make a backwards compatible change here.

 The current schema of the endpoint is list of objects where each object 
 has the keys:
 - penaltyMs (string)
 - taskIds (list of strings)
 - name (string)

 Surely we can add "reason" there as a list of strings.


 - Zameer

 On October 4th, 2016, 11:25 a.m. PDT, Pradyumna Kaushik wrote:
 Review request for Aurora and Joshua Cohen.
 By Pradyumna Kaushik.

 *Updated Oct. 4, 2016, 11:25 a.m.*
 *Repository: * aurora
 Description

 Added the 'reason' to the /pendingTasks endpoint

 Testing

 ./build-support/jenkins/build.sh
 ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh

 Diffs

- src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java
(c80e0c8adf80e12082a6952ae79b7d9cc960c5b6)
- src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java
(f783e7ff220573915524a1efc27141193d19fa6c)
- src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java
(5d319557057e27fd5fc6d3e553e9ca9139399c50)

 View Diff 

>>>
>>>
>>>
>>> --
>>> Thanks and Regards
>>> Pradyumna Kaushik
>>>
>>> *Graduate Student*
>>> *Department of Computer Science*
>>> *Thomas J. Watson School of Engineering and Applied Science*
>>> *Binghamton University*
>>>
>>
>>
>>
>> --
>> Zameer Manji
>>
>
>
>
> --
> Thanks and Regards
> Pradyumna Kaushik
>
> *Graduate Student*
> *Department of Computer Science*
> *Thomas J. Watson School of Engineering and Applied Science*
> *Binghamton University*
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-05 Thread Joshua Cohen 
+1, I was nervous about the mutability, but I mistakenly thought that the
tasks groups being used was request scoped. That's not the case though, so
we should definitely not be mutating it.

On Wed, Oct 5, 2016 at 2:21 PM, Zameer Manji  wrote:

> If you don't think it's too much work, I think Stephan's idea is ideal.
>
> The mutability of `reason` can result in bugs later.
>
> On Wed, Oct 5, 2016 at 11:31 AM, Pradyumna Kaushik <
> pkaus...@binghamton.edu>
> wrote:
>
> > Hi,
> >
> > Should I go ahead and work on Stephan's idea of returning a Map > String>  or should I hold it for now until the idea is a little more
> clear?
> >
> > On Tue, Oct 4, 2016 at 5:18 PM, Zameer Manji  wrote:
> >
> >> This is an automatically generated e-mail. To reply, visit:
> >> https://reviews.apache.org/r/51993/
> >>
> >> On October 4th, 2016, 2:10 p.m. PDT, *Stephan Erb* wrote:
> >>
> >> This change will be backwards incompatible. For example, it will break
> the Prometheus Aurora exporter: https://github.com/tommyulfsparre/aurora_
> exporter/blob/master/main.go#L101
> >>
> >> So the question: Do we consider this endpoint to be part of our public
> API? If yes, how can we remain backwards compatible?
> >>
> >> I'm not sure if we do make this endpoint apart of our public API, but I
> think we can make a backwards compatible change here.
> >>
> >> The current schema of the endpoint is list of objects where each object
> has the keys:
> >> - penaltyMs (string)
> >> - taskIds (list of strings)
> >> - name (string)
> >>
> >> Surely we can add "reason" there as a list of strings.
> >>
> >>
> >> - Zameer
> >>
> >> On October 4th, 2016, 11:25 a.m. PDT, Pradyumna Kaushik wrote:
> >> Review request for Aurora and Joshua Cohen.
> >> By Pradyumna Kaushik.
> >>
> >> *Updated Oct. 4, 2016, 11:25 a.m.*
> >> *Repository: * aurora
> >> Description
> >>
> >> Added the 'reason' to the /pendingTasks endpoint
> >>
> >> Testing
> >>
> >> ./build-support/jenkins/build.sh
> >> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> >>
> >> Diffs
> >>
> >>- src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java
> >>(c80e0c8adf80e12082a6952ae79b7d9cc960c5b6)
> >>- src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java
> >>(f783e7ff220573915524a1efc27141193d19fa6c)
> >>- src/main/java/org/apache/aurora/scheduler/scheduling/
> TaskGroup.java
> >>(5d319557057e27fd5fc6d3e553e9ca9139399c50)
> >>
> >> View Diff 
> >>
> >
> >
> >
> > --
> > Thanks and Regards
> > Pradyumna Kaushik
> >
> > *Graduate Student*
> > *Department of Computer Science*
> > *Thomas J. Watson School of Engineering and Applied Science*
> > *Binghamton University*
> >
>
>
>
> --
> Zameer Manji
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-05 Thread Zameer Manji 
If you don't think it's too much work, I think Stephan's idea is ideal.

The mutability of `reason` can result in bugs later.

On Wed, Oct 5, 2016 at 11:31 AM, Pradyumna Kaushik 
wrote:

> Hi,
>
> Should I go ahead and work on Stephan's idea of returning a Map String>  or should I hold it for now until the idea is a little more clear?
>
> On Tue, Oct 4, 2016 at 5:18 PM, Zameer Manji  wrote:
>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/51993/
>>
>> On October 4th, 2016, 2:10 p.m. PDT, *Stephan Erb* wrote:
>>
>> This change will be backwards incompatible. For example, it will break the 
>> Prometheus Aurora exporter: 
>> https://github.com/tommyulfsparre/aurora_exporter/blob/master/main.go#L101
>>
>> So the question: Do we consider this endpoint to be part of our public API? 
>> If yes, how can we remain backwards compatible?
>>
>> I'm not sure if we do make this endpoint apart of our public API, but I 
>> think we can make a backwards compatible change here.
>>
>> The current schema of the endpoint is list of objects where each object has 
>> the keys:
>> - penaltyMs (string)
>> - taskIds (list of strings)
>> - name (string)
>>
>> Surely we can add "reason" there as a list of strings.
>>
>>
>> - Zameer
>>
>> On October 4th, 2016, 11:25 a.m. PDT, Pradyumna Kaushik wrote:
>> Review request for Aurora and Joshua Cohen.
>> By Pradyumna Kaushik.
>>
>> *Updated Oct. 4, 2016, 11:25 a.m.*
>> *Repository: * aurora
>> Description
>>
>> Added the 'reason' to the /pendingTasks endpoint
>>
>> Testing
>>
>> ./build-support/jenkins/build.sh
>> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>>
>> Diffs
>>
>>- src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java
>>(c80e0c8adf80e12082a6952ae79b7d9cc960c5b6)
>>- src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java
>>(f783e7ff220573915524a1efc27141193d19fa6c)
>>- src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java
>>(5d319557057e27fd5fc6d3e553e9ca9139399c50)
>>
>> View Diff 
>>
>
>
>
> --
> Thanks and Regards
> Pradyumna Kaushik
>
> *Graduate Student*
> *Department of Computer Science*
> *Thomas J. Watson School of Engineering and Applied Science*
> *Binghamton University*
>



-- 
Zameer Manji

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-04 Thread Zameer Manji 


> On Oct. 4, 2016, 2:10 p.m., Stephan Erb wrote:
> > This change will be backwards incompatible. For example, it will break the 
> > Prometheus Aurora exporter: 
> > https://github.com/tommyulfsparre/aurora_exporter/blob/master/main.go#L101
> > 
> > So the question: Do we consider this endpoint to be part of our public API? 
> > If yes, how can we remain backwards compatible?

I'm not sure if we do make this endpoint apart of our public API, but I think 
we can make a backwards compatible change here.

The current schema of the endpoint is list of objects where each object has the 
keys:
- penaltyMs (string)
- taskIds (list of strings)
- name (string)

Surely we can add "reason" there as a list of strings.


- Zameer


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review151391
---


On Oct. 4, 2016, 11:25 a.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 4, 2016, 11:25 a.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-04 Thread Stephan Erb 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review151391
---



This change will be backwards incompatible. For example, it will break the 
Prometheus Aurora exporter: 
https://github.com/tommyulfsparre/aurora_exporter/blob/master/main.go#L101

So the question: Do we consider this endpoint to be part of our public API? If 
yes, how can we remain backwards compatible?


src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java (lines 32 - 
40)


I feel like we should not add any `reason` attribute to `TaskGroup`. It is 
mutable and only used in a subset of code paths where TaskGroups are used.

You could make this work by changing 
`nearestFit.addPendingReason(taskGroups.getGroups())` to be a 
`nearestFit.getPendingReason(taskGroups.getGroups())` that returns a 
`map` (where String is the reason).


- Stephan Erb


On Oct. 4, 2016, 8:25 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 4, 2016, 8:25 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-04 Thread Joshua Cohen 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review151341
---



The general approach looks reasonable to me. Please address the comments below 
and also, please add test coverage for the changes to the `PendingTasks` 
servlet as well as for the new method in `NearestFit`.


src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (line 126)


Add a newline as a separator between the method description and the 
parameter list (for consistency with other javadoc throughout the code)



src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (lines 129 - 
130)


Maybe just have this return `void` so it's clear that the `TaskGroup`'s 
will be modified in place?



src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (lines 129 - 
130)


This fits on one line.



src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (line 131)


Formatting should be:

// Iterating over ...

(i.e. space after the slashes, sentence capitalization).

That said, this comment is of questionable value, so I'd be fine with 
dropping it entirely...



src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (lines 135 - 
137)


Indentation is off here, should be +4 chars for continuation indents. That 
said, even cleaner would just be to inline this directly into the `setReason` 
call below.



src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java (line 43)


Making this method public is what's causing Jackson to fail to serialize 
this and necessitating your use of Gson in the PendingTasks servlet. You can 
avoid that be annotating this method with `@JsonIgnore` (but make sure you use 
the annotation from `org.codehaus...` not `com.fasterxml...`

With that done, the return from `PendingTasks#getOffers` should be able to 
revert back to `Response.ok(taskGroups.getGroups()).build()` (you'll still need 
to call `addPendingReasong` beforehand of course).


- Joshua Cohen


On Oct. 4, 2016, 3:20 a.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 4, 2016, 3:20 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-03 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review151293
---



Master (ed72b1b) 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 Oct. 4, 2016, 3:20 a.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Oct. 4, 2016, 3:20 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
> f783e7ff220573915524a1efc27141193d19fa6c 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-10-03 Thread Pradyumna Kaushik 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/
---

(Updated Oct. 4, 2016, 3:20 a.m.)


Review request for Aurora and Maxim Khutornenko.


Changes
---

Added the 'reason' to the /pendingTasks endpoint. Made addPendingReason(...) a 
public method of NearestFit, as was suggested before and TaskGroups.java no 
longer accesses NearestFit functionality. I am using com.google.Gson to 
marshall the results into a JSON. I know this is not the ideal solution but I 
was getting the following exception during marshalling,

```codehaus.JSONMappingException: union is current set to ...```

, possibly due to a conflict between fasterxml and codehaus, and tried really 
hard to fix it but unfortunately was not able to. I wanted to get this patch 
out soon and hence, I created the workaround. I would love it if someone could 
give me a tip to fix the issue so that I can then incorporate that and submit 
another patch.


Repository: aurora


Description
---

Added the 'reason' to the /pendingTasks endpoint


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
  src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
f783e7ff220573915524a1efc27141193d19fa6c 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
5d319557057e27fd5fc6d3e553e9ca9139399c50 

Diff: https://reviews.apache.org/r/51993/diff/


Testing
---

./build-support/jenkins/build.sh
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh


Thanks,

Pradyumna Kaushik

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-09-19 Thread Maxim Khutornenko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review149487
---




src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java (line 138)


I don't think this is the right approach. `TaskGroups` has no business in 
accessing the `NearestFit` functionality. 

Please, consider extracting 
[this](https://github.com/apache/aurora/blob/795a2728c623c35bd509d582c24684a6921c74ad/src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java#L194-L208)
 and making it a public method of the `NearestFit` itself (e.g.: 
`getPendingReason(Query.Builder query)`. You can query that method and marry 
the results right within the `PendingTasks`.


- Maxim Khutornenko


On Sept. 17, 2016, 9:36 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Sept. 17, 2016, 9:36 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/test/java/org/apache/aurora/scheduler/scheduling/TaskGroupsTest.java 
> 88729626de5fa87b45472792c59cc0ff1ade3e93 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Re: Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-09-17 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/#review149348
---


Ship it!




Master (a87ad41) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Sept. 17, 2016, 9:36 p.m., Pradyumna Kaushik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51993/
> ---
> 
> (Updated Sept. 17, 2016, 9:36 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added the 'reason' to the /pendingTasks endpoint
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
> c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/test/java/org/apache/aurora/scheduler/scheduling/TaskGroupsTest.java 
> 88729626de5fa87b45472792c59cc0ff1ade3e93 
> 
> Diff: https://reviews.apache.org/r/51993/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Pradyumna Kaushik
> 
>

Review Request 51993: Added the 'reason' to the /pendingTasks endpoint

2016-09-17 Thread Pradyumna Kaushik 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51993/
---

Review request for Aurora.


Repository: aurora


Description
---

Added the 'reason' to the /pendingTasks endpoint


Diffs
-

  src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java 
c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
5d319557057e27fd5fc6d3e553e9ca9139399c50 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
d390c07522d22e43d79ce4370985f3643ef021ca 
  src/test/java/org/apache/aurora/scheduler/scheduling/TaskGroupsTest.java 
88729626de5fa87b45472792c59cc0ff1ade3e93 

Diff: https://reviews.apache.org/r/51993/diff/


Testing
---

./build-support/jenkins/build.sh
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh


Thanks,

Pradyumna Kaushik