Address code review comment for aurora-scheduler#401 story

lawwong1 · Jun 6, 2022 · 0048bd1 · 0048bd1
1 parent 1d31e94
commit 0048bd1
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 20 deletions.
diff --git a/src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java b/src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
@@ -262,8 +262,8 @@ public Iterable<HostOffer> getOrdered(TaskGroupKey groupKey, ResourceRequest res
       ScheduleRequest scheduleRequest = createRequest(goodOffers, resourceRequest, startTime);
       LOG.info("Sending request {}", scheduleRequest.jobKey);
       String responseStr = sendRequest(scheduleRequest);
-      orderedOffers = processResponse(goodOffers, responseStr);
-    } catch (IOException e) {
+      orderedOffers = processResponse(goodOffers, responseStr, badOffers.size());
+    } catch (Exception e) {
       LOG.error("Failed to schedule the task of {} using {} ",
           resourceRequest.getTask().getJob().toString(), endpoint, e);
       HttpOfferSetImpl.incrementFailureCount();
@@ -325,7 +325,7 @@ private String sendRequest(ScheduleRequest scheduleRequest) throws IOException {
     }
   }
 
-  List<HostOffer> processResponse(List<HostOffer> mOffers, String responseStr)
+  List<HostOffer> processResponse(List<HostOffer> mOffers, String responseStr, int badOfferSize)
       throws IOException {
     ScheduleResponse response = jsonMapper.readValue(responseStr, ScheduleResponse.class);
     LOG.info("Received {} offers", response.hosts.size());
@@ -348,7 +348,7 @@ List<HostOffer> processResponse(List<HostOffer> mOffers, String responseStr)
             .collect(Collectors.toList());
 
     //offSetDiff is the value of the different between Aurora offers and response offers
-    long offSetDiff = mOffers.size() - orderedOffers.size() + extraOffers.size();
+    long offSetDiff = mOffers.size() + badOfferSize - orderedOffers.size() + extraOffers.size();
     offerSetDiffList.add(offSetDiff);
     if (offSetDiff > 0) {
       LOG.warn("The number of different offers between the original and received offer sets is {}",

diff --git a/src/test/java/io/github/aurora/scheduler/offers/HttpOfferSetImplTest.java b/src/test/java/io/github/aurora/scheduler/offers/HttpOfferSetImplTest.java
@@ -142,7 +142,7 @@ public void testProcessResponse() throws IOException {
 
     List<HostOffer> mOffers = ImmutableList.copyOf(httpOfferSet.values());
 
-    List<HostOffer> sortedOffers = httpOfferSet.processResponse(mOffers, responseStr);
+    List<HostOffer> sortedOffers = httpOfferSet.processResponse(mOffers, responseStr, 0);
     assertEquals(sortedOffers.size(), 3);
     assertEquals(sortedOffers.get(0).getAttributes().getHost(), HOST_A);
     assertEquals(sortedOffers.get(1).getAttributes().getHost(), HOST_B);
@@ -153,7 +153,7 @@ public void testProcessResponse() throws IOException {
     responseStr = "{\"error\": \"\", \"hosts\": [\""
             + HOST_A + "\",\""
             + HOST_C + "\"]}";
-    sortedOffers = httpOfferSet.processResponse(mOffers, responseStr);
+    sortedOffers = httpOfferSet.processResponse(mOffers, responseStr, 0);
     assertEquals(sortedOffers.size(), 2);
     assertEquals(sortedOffers.get(0).getAttributes().getHost(), HOST_A);
     assertEquals(sortedOffers.get(1).getAttributes().getHost(), HOST_C);
@@ -165,7 +165,7 @@ public void testProcessResponse() throws IOException {
             + HOST_B + "\",\""
             + HOST_D + "\",\""
             + HOST_C + "\"]}";
-    sortedOffers = httpOfferSet.processResponse(mOffers, responseStr);
+    sortedOffers = httpOfferSet.processResponse(mOffers, responseStr, 0);
     assertEquals(sortedOffers.size(), 3);
     assertEquals(sortedOffers.get(0).getAttributes().getHost(), HOST_A);
     assertEquals(sortedOffers.get(1).getAttributes().getHost(), HOST_B);
@@ -177,19 +177,26 @@ public void testProcessResponse() throws IOException {
         + HOST_A + "\",\""
         + HOST_D + "\",\""
         + HOST_C + "\"]}";
-    sortedOffers = httpOfferSet.processResponse(mOffers, responseStr);
+    sortedOffers = httpOfferSet.processResponse(mOffers, responseStr, 0);
     assertEquals(sortedOffers.size(), 2);
     assertEquals(sortedOffers.get(0).getAttributes().getHost(), HOST_A);
     assertEquals(sortedOffers.get(1).getAttributes().getHost(), HOST_C);
     assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(3), 2);
 
+    // Test with 1 bad offer
+    sortedOffers = httpOfferSet.processResponse(mOffers, responseStr, 1);
+    assertEquals(sortedOffers.size(), 2);
+    assertEquals(sortedOffers.get(0).getAttributes().getHost(), HOST_A);
+    assertEquals(sortedOffers.get(1).getAttributes().getHost(), HOST_C);
+    assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(4), 3);
+
     responseStr = "{\"error\": \"Error\", \"hosts\": [\""
             + HOST_A + "\",\""
             + HOST_B + "\",\""
             + HOST_C + "\"]}";
     boolean isException = false;
     try {
-      httpOfferSet.processResponse(mOffers, responseStr);
+      httpOfferSet.processResponse(mOffers, responseStr, 0);
     } catch (IOException ioe) {
       isException = true;
     }
@@ -198,7 +205,7 @@ public void testProcessResponse() throws IOException {
     responseStr = "{\"error\": \"error\"}";
     isException = false;
     try {
-      httpOfferSet.processResponse(mOffers, responseStr);
+      httpOfferSet.processResponse(mOffers, responseStr, 0);
     } catch (IOException ioe) {
       isException = true;
     }
@@ -207,7 +214,7 @@ public void testProcessResponse() throws IOException {
     responseStr = "{\"weird\": \"cannot decode this json string\"}";
     isException = false;
     try {
-      httpOfferSet.processResponse(mOffers, responseStr);
+      httpOfferSet.processResponse(mOffers, responseStr, 0);
     } catch (IOException ioe) {
       isException = true;
     }
@@ -224,55 +231,62 @@ public void testProcessResponse() throws IOException {
     assertEquals(mDuplicateHostOffers.size(), 4);
 
     sortedOffers = duplicateHostsHttpOfferSet.processResponse(mDuplicateHostOffers,
-            responseStr);
+            responseStr, 0);
     assertEquals(sortedOffers.size(), 4);
     assertEquals(sortedOffers.get(0).getAttributes().getHost(), HOST_A);
     assertEquals(sortedOffers.get(1).getAttributes().getHost(), HOST_B);
     assertEquals(sortedOffers.get(2).getAttributes().getHost(), HOST_C);
     assertEquals(sortedOffers.get(3).getAttributes().getHost(), HOST_C);
-    assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(4), 0);
+    assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(5), 0);
 
     // plugin returns less offers than Aurora has.
     responseStr = "{\"error\": \"\", \"hosts\": [\""
             + HOST_A + "\",\""
             + HOST_C + "\"]}";
-    sortedOffers = duplicateHostsHttpOfferSet.processResponse(mDuplicateHostOffers, responseStr);
+    sortedOffers = duplicateHostsHttpOfferSet.processResponse(mDuplicateHostOffers, responseStr, 0);
     assertEquals(sortedOffers.size(), 3);
     assertEquals(sortedOffers.get(0).getAttributes().getHost(), HOST_A);
     assertEquals(sortedOffers.get(1).getAttributes().getHost(), HOST_C);
-    assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(5), 1);
+    assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(6), 1);
 
     // plugin returns more offers than Aurora has.
     responseStr = "{\"error\": \"\", \"hosts\": [\""
             + HOST_A + "\",\""
             + HOST_B + "\",\""
             + HOST_D + "\",\""
             + HOST_C + "\"]}";
-    sortedOffers = duplicateHostsHttpOfferSet.processResponse(mDuplicateHostOffers, responseStr);
+    sortedOffers = duplicateHostsHttpOfferSet.processResponse(mDuplicateHostOffers, responseStr, 0);
     assertEquals(sortedOffers.size(), 4);
     assertEquals(sortedOffers.get(0).getAttributes().getHost(), HOST_A);
     assertEquals(sortedOffers.get(1).getAttributes().getHost(), HOST_B);
     assertEquals(sortedOffers.get(2).getAttributes().getHost(), HOST_C);
-    assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(6), 1);
+    assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(7), 1);
 
     // plugin omits 1 offer & returns 1 extra offer
     responseStr = "{\"error\": \"\", \"hosts\": [\""
             + HOST_A + "\",\""
             + HOST_D + "\",\""
             + HOST_B + "\"]}";
-    sortedOffers = duplicateHostsHttpOfferSet.processResponse(mDuplicateHostOffers, responseStr);
+    sortedOffers = duplicateHostsHttpOfferSet.processResponse(mDuplicateHostOffers, responseStr, 0);
+    assertEquals(sortedOffers.size(), 2);
+    assertEquals(sortedOffers.get(0).getAttributes().getHost(), HOST_A);
+    assertEquals(sortedOffers.get(1).getAttributes().getHost(), HOST_B);
+    assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(8), 3);
+
+    // Test with 1 bad offer
+    sortedOffers = duplicateHostsHttpOfferSet.processResponse(mDuplicateHostOffers, responseStr, 1);
     assertEquals(sortedOffers.size(), 2);
     assertEquals(sortedOffers.get(0).getAttributes().getHost(), HOST_A);
     assertEquals(sortedOffers.get(1).getAttributes().getHost(), HOST_B);
-    assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(7), 3);
+    assertEquals((long) HttpOfferSetImpl.offerSetDiffList.get(9), 4);
 
     responseStr = "{\"error\": \"Error\", \"hosts\": [\""
             + HOST_A + "\",\""
             + HOST_B + "\",\""
             + HOST_C + "\"]}";
     isException = false;
     try {
-      duplicateHostsHttpOfferSet.processResponse(mOffers, responseStr);
+      duplicateHostsHttpOfferSet.processResponse(mOffers, responseStr, 0);
     } catch (IOException ioe) {
       isException = true;
     }