From 0048bd1e6e4a6ab19da00bfecc8c469825d799a5 Mon Sep 17 00:00:00 2001 From: Lawrence Wong Date: Mon, 6 Jun 2022 15:47:59 -0700 Subject: [PATCH] Address code review comment for #401 story --- .../scheduler/offers/HttpOfferSetImpl.java | 8 ++-- .../offers/HttpOfferSetImplTest.java | 46 ++++++++++++------- 2 files changed, 34 insertions(+), 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 index 737109822..50bc5d71d 100644 --- 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 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 processResponse(List mOffers, String responseStr) + List processResponse(List 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 processResponse(List 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 index cb13d03a3..6900e9b57 100644 --- 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 mOffers = ImmutableList.copyOf(httpOfferSet.values()); - List sortedOffers = httpOfferSet.processResponse(mOffers, responseStr); + List 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,23 +231,23 @@ 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\": [\"" @@ -248,23 +255,30 @@ public void testProcessResponse() throws IOException { + 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 + "\",\"" @@ -272,7 +286,7 @@ public void testProcessResponse() throws IOException { + HOST_C + "\"]}"; isException = false; try { - duplicateHostsHttpOfferSet.processResponse(mOffers, responseStr); + duplicateHostsHttpOfferSet.processResponse(mOffers, responseStr, 0); } catch (IOException ioe) { isException = true; }