Skip to content

Commit a8bd0b9

Browse files
patrick-austinpatrick-austin
authored
Merge pull request #88 from ral-facilities/persist_priority
Persist priority in Download, calculate at write not read
2 parents 70f19f6 + c2b0011 commit a8bd0b9

File tree

10 files changed

+76
-69
lines changed

10 files changed

+76
-69
lines changed

‎migrations/mysql_3.2.0.sql‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTER TABLE `DOWNLOAD` ADD `PRIORITY` TINYINT UNSIGNED DEFAULT 0 NOT NULL;

‎migrations/oracle_3.2.0.sql‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTER TABLE DOWNLOAD ADD PRIORITY NUMBER(2, 0) DEFAULT 0 NOT NULL;

‎pom.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
<groupId>org.icatproject</groupId>
66
<artifactId>datagateway-download-api</artifactId>
77
<packaging>war</packaging>
8-
<version>3.1.1-SNAPSHOT</version>
8+
<version>3.2.0-SNAPSHOT</version>
99
<name>DataGateway Download API</name>
1010
<description>Download backend for DataGateway</description>
1111

‎src/main/java/org/icatproject/topcat/IcatClient.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -411,7 +411,15 @@ public List<JsonObject> getEntities(String entityType, List<Long> entityIds) thr
411411
* another internal error is triggered)
412412
*/
413413
public void checkQueueAllowed(String userName) throws TopcatException {
414-
if (getQueuePriority(userName) < 1) {
414+
checkQueueAllowed(getQueuePriority(userName));
415+
}
416+
417+
/**
418+
* @param priority int priority to check
419+
* @throws TopcatException if priority < 1
420+
*/
421+
public void checkQueueAllowed(int priority) throws TopcatException {
422+
if (priority < 1) {
415423
throw new ForbiddenException("Queuing Downloads forbidden");
416424
}
417425
}

‎src/main/java/org/icatproject/topcat/StatusCheck.java‎

Lines changed: 5 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -397,8 +397,8 @@ public void startQueuedDownload(int maxActiveDownloads) throws Exception {
397397
availableDownloads -= activeDownloadsSize;
398398
}
399399

400-
String queuedQueryString = selectString + " and " + queuedCondition;
401-
queuedQueryString += " order by download.createdAt";
400+
String queuedQueryString = selectString + " and " + queuedCondition + " and download.priority > 0";
401+
queuedQueryString += " order by download.priority asc NULLS FIRST, download.createdAt asc";
402402
TypedQuery<Download> queuedDownloadsQuery = em.createQuery(queuedQueryString, Download.class);
403403
List<Download> queuedDownloads = queuedDownloadsQuery.getResultList();
404404
int queueSize = queuedDownloads.size();
@@ -410,42 +410,12 @@ public void startQueuedDownload(int maxActiveDownloads) throws Exception {
410410
if (maxActiveDownloads <= 0) {
411411
// No limits on how many to submit
412412
logger.info("Preparing 1 out of {} queued downloads", queueSize);
413-
Download queuedDownload = queuedDownloads.get(0);
414-
queuedDownload.setStatus(DownloadStatus.PREPARING);
415-
prepareDownload(queuedDownload, null, getQueueSessionId(sessionIds, queuedDownload.getFacilityName()));
416413
} else {
417414
logger.info("Preparing 1 out of {} queued downloads as {} spaces available", queueSize, availableDownloads);
418-
HashMap<Integer, List<Download>> mapping = new HashMap<>();
419-
for (Download queuedDownload : queuedDownloads) {
420-
String sessionId = getQueueSessionId(sessionIds, queuedDownload.getFacilityName());
421-
String icatUrl = FacilityMap.getInstance().getIcatUrl(queuedDownload.getFacilityName());
422-
IcatClient icatClient = new IcatClient(icatUrl, sessionId);
423-
int priority = icatClient.getQueuePriority(queuedDownload.getUserName());
424-
if (priority == 1) {
425-
// Highest priority, prepare now
426-
queuedDownload.setStatus(DownloadStatus.PREPARING);
427-
prepareDownload(queuedDownload, null, sessionId);
428-
return;
429-
} else {
430-
// Lower priority, add to mapping
431-
mapping.putIfAbsent(priority, new ArrayList<>());
432-
mapping.get(priority).add(queuedDownload);
433-
}
434-
}
435-
436-
// Get the highest priority encountered
437-
List<Integer> keyList = new ArrayList<>();
438-
for (Object key : mapping.keySet().toArray()) {
439-
keyList.add((Integer) key);
440-
}
441-
int priority = Collections.min(keyList);
442-
443-
// Prepare the first Download at this priority level
444-
List<Download> downloadList = mapping.get(priority);
445-
Download download = downloadList.get(0);
446-
download.setStatus(DownloadStatus.PREPARING);
447-
prepareDownload(download, null, getQueueSessionId(sessionIds, download.getFacilityName()));
448415
}
416+
Download queuedDownload = queuedDownloads.get(0);
417+
queuedDownload.setStatus(DownloadStatus.PREPARING);
418+
prepareDownload(queuedDownload, null, getQueueSessionId(sessionIds, queuedDownload.getFacilityName()));
449419
}
450420

451421
private void handleException( Download download, String reason, boolean doExpire ) {

‎src/main/java/org/icatproject/topcat/domain/Download.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,9 @@ public class Download implements Serializable {
7373
@Enumerated(EnumType.STRING)
7474
private DownloadStatus status;
7575

76+
@Column(name = "PRIORITY")
77+
private Integer priority;
78+
7679
@Column(name = "THE_SIZE")
7780
private long size;
7881

@@ -188,6 +191,14 @@ public void setStatus(DownloadStatus status) {
188191
this.status = status;
189192
}
190193

194+
public int getPriority() {
195+
return priority;
196+
}
197+
198+
public void setPriority(int priority) {
199+
this.priority = priority;
200+
}
201+
191202
public long getSize() {
192203
return size;
193204
}

‎src/main/java/org/icatproject/topcat/web/rest/UserResource.java‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -762,7 +762,7 @@ public Response submitCart(@PathParam("facilityName") String facilityName,
762762
downloadItems.add(downloadItem);
763763
}
764764
download.setDownloadItems(downloadItems);
765-
downloadId = submitDownload(idsClient, download, DownloadStatus.PREPARING);
765+
downloadId = submitDownload(idsClient, download, DownloadStatus.PREPARING, 0);
766766
try {
767767
em.remove(cart);
768768
em.flush();
@@ -824,11 +824,14 @@ private static DownloadItem createDownloadItem(Download download, long entityId,
824824
* @param idsClient Client for the IDS to use for the Download
825825
* @param download Download to submit
826826
* @param downloadStatus Initial DownloadStatus to set if and only if the IDS isTwoLevel
827+
* @param priority Priority to submit the Download with
827828
* @return Id of the new Download
828829
* @throws TopcatException
829830
*/
830-
private long submitDownload(IdsClient idsClient, Download download, DownloadStatus downloadStatus)
831+
private long submitDownload(IdsClient idsClient, Download download, DownloadStatus downloadStatus, int priority)
831832
throws TopcatException {
833+
834+
download.setPriority(priority);
832835
Boolean isTwoLevel = idsClient.isTwoLevel();
833836
download.setIsTwoLevel(isTwoLevel);
834837

@@ -890,7 +893,8 @@ public Response queueVisitId(@FormParam("facilityName") String facilityName,
890893
// If we wanted to block the user, this is where we would do it
891894
String userName = icatClient.getUserName();
892895
String fullName = icatClient.getFullName();
893-
icatClient.checkQueueAllowed(userName);
896+
int priority = icatClient.getQueuePriority(userName);
897+
icatClient.checkQueueAllowed(priority);
894898
JsonArray datasets = icatClient.getDatasets(visitId);
895899
if (datasets.size() == 0) {
896900
throw new NotFoundException("No Datasets found for " + visitId);
@@ -945,7 +949,7 @@ public Response queueVisitId(@FormParam("facilityName") String facilityName,
945949
for (Download download : downloads) {
946950
String partFilename = formatQueuedFilename(fileName, part, downloads.size());
947951
download.setFileName(partFilename);
948-
downloadId = submitDownload(idsClient, download, DownloadStatus.QUEUED);
952+
downloadId = submitDownload(idsClient, download, DownloadStatus.QUEUED, priority);
949953
jsonArrayBuilder.add(downloadId);
950954
part += 1;
951955
}
@@ -1017,7 +1021,8 @@ public Response queueFiles(@FormParam("facilityName") String facilityName,
10171021

10181022
String userName = icatClient.getUserName();
10191023
String fullName = icatClient.getFullName();
1020-
icatClient.checkQueueAllowed(userName);
1024+
int priority = icatClient.getQueuePriority(userName);
1025+
icatClient.checkQueueAllowed(priority);
10211026

10221027
DatafilesResponse response = icatClient.getDatafiles(files);
10231028
if (response.ids.size() == 0) {
@@ -1033,7 +1038,7 @@ public Response queueFiles(@FormParam("facilityName") String facilityName,
10331038
download.setDownloadItems(downloadItems);
10341039
download.setSize(response.totalSize);
10351040

1036-
long downloadId = submitDownload(idsClient, download, DownloadStatus.QUEUED);
1041+
long downloadId = submitDownload(idsClient, download, DownloadStatus.QUEUED, priority);
10371042

10381043
JsonObjectBuilder jsonObjectBuilder = Json.createObjectBuilder();
10391044
JsonArrayBuilder jsonArrayBuilder = Json.createArrayBuilder();

‎src/test/java/org/icatproject/topcat/StatusCheckTest.java‎

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -562,7 +562,7 @@ public void testExpiredDownloadsIgnored() throws Exception {
562562

563563
// Create a single-tier download; initial status should be COMPLETE
564564
Download dummyDownload = TestHelpers.createDummyDownload("DummyUserName", preparedId, transport, false,
565-
status, false, downloadRepository);
565+
status, 0, false, downloadRepository);
566566
downloadId = dummyDownload.getId();
567567

568568
// Not testing delays, so set to zero
@@ -764,28 +764,44 @@ public void testStartQueuedDownloadNegative() throws Exception {
764764
System.out.println("DEBUG testStartQueuedDownloadNegative");
765765
Long downloadId1 = null;
766766
Long downloadId2 = null;
767+
Long downloadId3 = null;
768+
Long downloadId4 = null;
767769
try {
768770
String transport = "http";
769771
Download dummyDownload1 = TestHelpers.createDummyDownload("DummyUserName", null, transport, true,
770-
DownloadStatus.QUEUED, false, downloadRepository);
772+
DownloadStatus.QUEUED, 2, false, downloadRepository);
771773
Download dummyDownload2 = TestHelpers.createDummyDownload("DummyUserName", null, transport, true,
772-
DownloadStatus.QUEUED, false, downloadRepository);
774+
DownloadStatus.QUEUED, 1, false, downloadRepository);
775+
Download dummyDownload3 = TestHelpers.createDummyDownload("DummyUserName", null, transport, true,
776+
DownloadStatus.QUEUED, 2, false, downloadRepository);
777+
Download dummyDownload4 = TestHelpers.createDummyDownload("DummyUserName", null, transport, true,
778+
DownloadStatus.QUEUED, 1, false, downloadRepository);
773779
downloadId1 = dummyDownload1.getId();
774780
downloadId2 = dummyDownload2.getId();
781+
downloadId3 = dummyDownload3.getId();
782+
downloadId4 = dummyDownload4.getId();
775783

776784
statusCheck.startQueuedDownload(-1);
777785

778786
Download postDownload1 = TestHelpers.getDummyDownload(downloadId1, downloadRepository);
779787
Download postDownload2 = TestHelpers.getDummyDownload(downloadId2, downloadRepository);
780-
781-
assertEquals(DownloadStatus.RESTORING, postDownload1.getStatus());
782-
assertNotNull(postDownload1.getPreparedId());
783-
assertEquals(DownloadStatus.QUEUED, postDownload2.getStatus());
784-
assertNull(postDownload2.getPreparedId());
788+
Download postDownload3 = TestHelpers.getDummyDownload(downloadId3, downloadRepository);
789+
Download postDownload4 = TestHelpers.getDummyDownload(downloadId4, downloadRepository);
790+
791+
assertEquals(DownloadStatus.QUEUED, postDownload1.getStatus());
792+
assertEquals(DownloadStatus.RESTORING, postDownload2.getStatus());
793+
assertEquals(DownloadStatus.QUEUED, postDownload3.getStatus());
794+
assertEquals(DownloadStatus.QUEUED, postDownload4.getStatus());
795+
assertNull(postDownload1.getPreparedId());
796+
assertNotNull(postDownload2.getPreparedId());
797+
assertNull(postDownload3.getPreparedId());
798+
assertNull(postDownload4.getPreparedId());
785799
} finally {
786800
// clean up
787801
TestHelpers.deleteDummyDownload(downloadId1, downloadRepository);
788802
TestHelpers.deleteDummyDownload(downloadId2, downloadRepository);
803+
TestHelpers.deleteDummyDownload(downloadId3, downloadRepository);
804+
TestHelpers.deleteDummyDownload(downloadId4, downloadRepository);
789805
}
790806
}
791807

@@ -796,7 +812,7 @@ public void testStartQueuedDownloadZero() throws Exception {
796812
try {
797813
String transport = "http";
798814
Download dummyDownload = TestHelpers.createDummyDownload("DummyUserName", null, transport, true,
799-
DownloadStatus.QUEUED, false, downloadRepository);
815+
DownloadStatus.QUEUED, 1, false, downloadRepository);
800816
downloadId = dummyDownload.getId();
801817

802818
statusCheck.startQueuedDownload(0);
@@ -822,9 +838,9 @@ public void testStartQueuedDownloadNonZero() throws Exception {
822838
try {
823839
String transport = "http";
824840
Download dummyDownload1 = TestHelpers.createDummyDownload("DummyUserName", null, transport, true,
825-
DownloadStatus.QUEUED, false, downloadRepository);
841+
DownloadStatus.QUEUED, 1, false, downloadRepository);
826842
Download dummyDownload2 = TestHelpers.createDummyDownload("DummyUserName", null, transport, true,
827-
DownloadStatus.QUEUED, false, downloadRepository);
843+
DownloadStatus.QUEUED, 1, false, downloadRepository);
828844
downloadId1 = dummyDownload1.getId();
829845
downloadId2 = dummyDownload2.getId();
830846

@@ -852,9 +868,9 @@ public void testStartQueuedDownloadNonZeroRestoringDownload() throws Exception {
852868
try {
853869
String transport = "http";
854870
Download dummyDownload1 = TestHelpers.createDummyDownload("DummyUserName", "preparedId", transport, true,
855-
DownloadStatus.RESTORING, false, downloadRepository);
871+
DownloadStatus.RESTORING, 0, false, downloadRepository);
856872
Download dummyDownload2 = TestHelpers.createDummyDownload("DummyUserName", null, transport, true,
857-
DownloadStatus.QUEUED, false, downloadRepository);
873+
DownloadStatus.QUEUED, 1, false, downloadRepository);
858874
downloadId1 = dummyDownload1.getId();
859875
downloadId2 = dummyDownload2.getId();
860876

@@ -880,10 +896,10 @@ public void testStartQueuedDownloadNonZeroRestoringDownload() throws Exception {
880896
private Download createDummyDownload(String preparedId, String transport, Boolean isTwoLevel, Boolean isDeleted) {
881897
if (isTwoLevel) {
882898
return TestHelpers.createDummyDownload("DummyUserName", preparedId, transport, isTwoLevel,
883-
DownloadStatus.PREPARING, isDeleted, downloadRepository);
899+
DownloadStatus.PREPARING, 0, isDeleted, downloadRepository);
884900
} else {
885901
return TestHelpers.createDummyDownload("DummyUserName", preparedId, transport, isTwoLevel,
886-
DownloadStatus.COMPLETE, isDeleted, downloadRepository);
902+
DownloadStatus.COMPLETE, 0, isDeleted, downloadRepository);
887903
}
888904
}
889905
}

‎src/test/java/org/icatproject/topcat/TestHelpers.java‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public boolean verify(String hostname, SSLSession session) {
5959
}
6060

6161
public static Download createDummyDownload(String userName, String preparedId, String transport, Boolean isTwoLevel,
62-
DownloadStatus downloadStatus, Boolean isDeleted, DownloadRepository downloadRepository) {
62+
DownloadStatus downloadStatus, int priority, Boolean isDeleted, DownloadRepository downloadRepository) {
6363

6464
// This mocks what UserResource.submitCart() might do.
6565

@@ -86,14 +86,9 @@ public static Download createDummyDownload(String userName, String preparedId, S
8686

8787
List<DownloadItem> downloadItems = new ArrayList<DownloadItem>();
8888
download.setDownloadItems(downloadItems);
89-
9089
download.setIsTwoLevel(isTwoLevel);
91-
92-
if (isTwoLevel) {
93-
download.setStatus(downloadStatus);
94-
} else {
95-
download.setStatus(downloadStatus);
96-
}
90+
download.setStatus(downloadStatus);
91+
download.setPriority(priority);
9792

9893
return downloadRepository.save(download);
9994
}

‎src/test/java/org/icatproject/topcat/UserResourceTest.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -512,7 +512,7 @@ public void testGetDownloadNotFound() throws MalformedURLException, TopcatExcept
512512
List<Long> downloadIds = new ArrayList<>();
513513
try {
514514
Download download = TestHelpers.createDummyDownload("simple/notroot", null, "http", true,
515-
DownloadStatus.COMPLETE, false, downloadRepository);
515+
DownloadStatus.COMPLETE, 0, false, downloadRepository);
516516

517517
downloadIds.add(download.getId());
518518
Executable runnable = () -> userResource.getDownloadStatuses("LILS", sessionId, downloadIds);
@@ -529,9 +529,9 @@ public void testGetDownloadStatuses() throws MalformedURLException, TopcatExcept
529529
List<Long> downloadIds = new ArrayList<>();
530530
try {
531531
Download download1 = TestHelpers.createDummyDownload("simple/root", null, "http", true,
532-
DownloadStatus.COMPLETE, false, downloadRepository);
532+
DownloadStatus.COMPLETE, 0, false, downloadRepository);
533533
Download download2 = TestHelpers.createDummyDownload("simple/root", null, "http", true,
534-
DownloadStatus.RESTORING, false, downloadRepository);
534+
DownloadStatus.RESTORING, 0, false, downloadRepository);
535535

536536
downloadIds.add(download1.getId());
537537
downloadIds.add(download2.getId());

0 commit comments

Comments
 (0)