From f4e15e4a3f4bcbdf7da25ae10bc48c1bf46ca4c8 Mon Sep 17 00:00:00 2001 From: rtirabassi Date: Wed, 12 Feb 2020 12:25:26 +0100 Subject: [PATCH 1/3] fix: removed double keycloak declaration in pom.xml add: set UIDMap cache into LocationQuery and same reference usage in order to save RAM. --- dcm4chee-arc-audit/pom.xml | 6 ------ .../org/dcm4chee/arc/retrieve/RetrieveService.java | 7 +++++-- .../dcm4chee/arc/retrieve/impl/LocationQuery.java | 9 +++++---- .../arc/retrieve/impl/RetrieveContextImpl.java | 7 +++++-- .../arc/retrieve/impl/RetrieveServiceEJB.java | 11 +++++++++++ .../arc/retrieve/impl/RetrieveServiceImpl.java | 13 +++++++++---- 6 files changed, 35 insertions(+), 18 deletions(-) diff --git a/dcm4chee-arc-audit/pom.xml b/dcm4chee-arc-audit/pom.xml index d7bb5ef8d8..02764164b5 100644 --- a/dcm4chee-arc-audit/pom.xml +++ b/dcm4chee-arc-audit/pom.xml @@ -82,12 +82,6 @@ ${project.version} provided - - org.dcm4che.dcm4chee-arc - dcm4chee-arc-keycloak - ${project.version} - provided - org.dcm4che.dcm4chee-arc dcm4chee-arc-patient diff --git a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/RetrieveService.java b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/RetrieveService.java index 789e546cf3..c64e0bf600 100644 --- a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/RetrieveService.java +++ b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/RetrieveService.java @@ -53,8 +53,9 @@ import org.dcm4chee.arc.conf.ArchiveAEExtension; import org.dcm4chee.arc.conf.ArchiveDeviceExtension; import org.dcm4chee.arc.entity.Series; -import org.dcm4chee.arc.metrics.MetricsService; +import org.dcm4chee.arc.entity.UIDMap; import org.dcm4chee.arc.keycloak.HttpServletRequestInfo; +import org.dcm4chee.arc.metrics.MetricsService; import org.dcm4chee.arc.storage.Storage; import org.dcm4chee.arc.store.InstanceLocations; import org.dcm4chee.arc.store.StoreService; @@ -85,7 +86,7 @@ RetrieveContext newRetrieveContextGET(ArchiveAEExtension arcAE, Association as, Attributes cmd, QueryRetrieveLevel2 qrLevel, Attributes keys); RetrieveContext newRetrieveContextMOVE(ArchiveAEExtension arcAE, - Association as, Attributes cmd, QueryRetrieveLevel2 qrLevel, Attributes keys) + Association as, Attributes cmd, QueryRetrieveLevel2 qrLevel, Attributes keys) throws ConfigurationException; RetrieveContext newRetrieveContextWADO( @@ -149,4 +150,6 @@ LocationInputStream openLocationInputStream(RetrieveContext ctx, InstanceLocatio Date getLastModifiedFromMatches(RetrieveContext ctx); Date getLastModified(RetrieveContext ctx); + + UIDMap getUIDMap(Long uidMapPk, Map uidMapCache); } diff --git a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/LocationQuery.java b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/LocationQuery.java index f861f84728..537348060e 100644 --- a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/LocationQuery.java +++ b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/LocationQuery.java @@ -55,7 +55,6 @@ import org.hibernate.engine.spi.SessionFactoryImplementor; import javax.persistence.EntityManager; -import javax.persistence.NoResultException; import javax.persistence.Tuple; import javax.persistence.criteria.*; import java.util.ArrayList; @@ -82,6 +81,8 @@ class LocationQuery { private final Path instanceAttrBlob; private final List predicates = new ArrayList<>(); private Predicate[] iuidPredicates; + private final HashMap uidMapCache = new HashMap<>(); + public LocationQuery(EntityManager em, CriteriaBuilder cb, RetrieveContext ctx, CodeCache codeCache) { this.em = em; @@ -146,7 +147,7 @@ public LocationQuery(EntityManager em, CriteriaBuilder cb, RetrieveContext ctx, instance.get(Instance_.createdTime), instance.get(Instance_.updatedTime), uidMap.get(UIDMap_.pk), - uidMap.get(UIDMap_.encodedMap), + // uidMap.get(UIDMap_.encodedMap), instanceAttrBlob ); } @@ -171,7 +172,7 @@ public void execute(Map studyInfoMap) { } private void execute(Map studyInfoMap, Predicate[] predicates) { - HashMap instMap = new HashMap<>(); + HashMap instMap = new HashMap<>(); HashMap seriesAttrsMap = new HashMap<>(); HashMap> rejectedInstancesOfSeriesMap = new HashMap<>(); for (Tuple tuple : em.createQuery(q.where(predicates)).getResultList()) { @@ -242,7 +243,7 @@ private void addLocation(InstanceLocations match, Tuple tuple) { .build(); Long uidMapPk = tuple.get(uidMap.get(UIDMap_.pk)); if (uidMapPk != null) { - location.setUidMap(new UIDMap(uidMapPk, tuple.get(uidMap.get(UIDMap_.encodedMap)))); + location.setUidMap(ctx.getRetrieveService().getUIDMap(uidMapPk, uidMapCache)); } match.getLocations().add(location); } diff --git a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveContextImpl.java b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveContextImpl.java index d85c98b1d3..5b16952a21 100644 --- a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveContextImpl.java +++ b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveContextImpl.java @@ -52,9 +52,12 @@ import org.dcm4chee.arc.conf.*; import org.dcm4chee.arc.entity.Location; import org.dcm4chee.arc.entity.Series; -import org.dcm4chee.arc.retrieve.*; -import org.dcm4chee.arc.storage.Storage; import org.dcm4chee.arc.keycloak.HttpServletRequestInfo; +import org.dcm4chee.arc.retrieve.RetrieveContext; +import org.dcm4chee.arc.retrieve.RetrieveService; +import org.dcm4chee.arc.retrieve.SeriesInfo; +import org.dcm4chee.arc.retrieve.StudyInfo; +import org.dcm4chee.arc.storage.Storage; import org.dcm4chee.arc.store.InstanceLocations; import org.dcm4chee.arc.store.UpdateLocation; diff --git a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveServiceEJB.java b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveServiceEJB.java index d8c7f37771..c4c322e742 100644 --- a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveServiceEJB.java +++ b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveServiceEJB.java @@ -43,11 +43,13 @@ import org.dcm4chee.arc.entity.Completeness; import org.dcm4chee.arc.entity.Series; import org.dcm4chee.arc.entity.Study; +import org.dcm4chee.arc.entity.UIDMap; import org.dcm4chee.arc.retrieve.RetrieveContext; import javax.ejb.Stateless; import javax.persistence.EntityManager; import javax.persistence.PersistenceContext; +import java.util.Map; /** * @author Gunter Zeilinger @@ -87,6 +89,15 @@ public void updateCompleteness(RetrieveContext ctx, Completeness completeness) { } } + public UIDMap getUIDMapReference(Long uidMapPk, Map uidMapCache) { + UIDMap uidMap = uidMapCache.get(uidMapPk) ; + if ( null == uidMap ) { + uidMap = em.find(UIDMap.class, uidMapPk) ; + uidMapCache.put(uidMapPk, uidMap) ; + } + return uidMap; + } + private void setCompletenessOfStudy(String studyInstanceUID, Completeness completeness) { em.createNamedQuery(completeness == Completeness.PARTIAL ? Study.INCREMENT_FAILED_RETRIEVES diff --git a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveServiceImpl.java b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveServiceImpl.java index d0fe6f8b20..bb85293b58 100644 --- a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveServiceImpl.java +++ b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/RetrieveServiceImpl.java @@ -58,8 +58,8 @@ import org.dcm4chee.arc.code.CodeCache; import org.dcm4chee.arc.conf.*; import org.dcm4chee.arc.entity.*; -import org.dcm4chee.arc.metrics.MetricsService; import org.dcm4chee.arc.keycloak.HttpServletRequestInfo; +import org.dcm4chee.arc.metrics.MetricsService; import org.dcm4chee.arc.query.scu.CFindSCU; import org.dcm4chee.arc.query.scu.CFindSCUAttributeCoercion; import org.dcm4chee.arc.query.util.QueryBuilder; @@ -153,7 +153,7 @@ public ArchiveDeviceExtension getArchiveDeviceExtension() { @Override public RetrieveContext newRetrieveContextGET(ArchiveAEExtension arcAE, - Association as, Attributes rqCmd, QueryRetrieveLevel2 qrLevel, Attributes keys) { + Association as, Attributes rqCmd, QueryRetrieveLevel2 qrLevel, Attributes keys) { RetrieveContext ctx = newRetrieveContext(arcAE, as, qrLevel, keys); ctx.setPriority(rqCmd.getInt(Tag.Priority, 0)); ctx.setDestinationAETitle(as.getRemoteAET()); @@ -162,7 +162,7 @@ public RetrieveContext newRetrieveContextGET(ArchiveAEExtension arcAE, @Override public RetrieveContext newRetrieveContextMOVE(ArchiveAEExtension arcAE, - Association as, Attributes rqCmd, QueryRetrieveLevel2 qrLevel, Attributes keys) + Association as, Attributes rqCmd, QueryRetrieveLevel2 qrLevel, Attributes keys) throws ConfigurationException { RetrieveContext ctx = newRetrieveContext(arcAE, as, qrLevel, keys); ctx.setPriority(rqCmd.getInt(Tag.Priority, 0)); @@ -321,7 +321,7 @@ public boolean calculateMatches(RetrieveContext ctx) throws DicomServiceExceptio Collection matches = ctx.getMatches(); matches.clear(); try { - HashMap studyInfoMap = new HashMap<>(); + HashMap studyInfoMap = new HashMap<>(); Series.MetadataUpdate metadataUpdate = ctx.getSeriesMetadataUpdate(); if (metadataUpdate != null && metadataUpdate.instancePurgeState == Series.InstancePurgeState.PURGED) { SeriesAttributes seriesAttributes = new SeriesAttributes(em, cb, metadataUpdate.seriesPk); @@ -982,4 +982,9 @@ public BulkData createBulkData(DicomInputStream dis) throws IOException { } return attrs; } + + @Override + public UIDMap getUIDMap(Long uidMapPk, Map uidMapCache) { + return ejb.getUIDMapReference(uidMapPk, uidMapCache); + } } From 5a005ca1e9fe5b00d66591e83344b7ba133ce560 Mon Sep 17 00:00:00 2001 From: rtirabassi Date: Wed, 12 Feb 2020 17:29:20 +0100 Subject: [PATCH 2/3] fix: Deletion of files from storage is now safer. Location is removed when asked, but file is not if still in use. --- .../org/dcm4chee/arc/delete/impl/DeletionServiceEJB.java | 8 ++++++++ .../dcm4chee/arc/delete/impl/PurgeStorageScheduler.java | 8 +++++++- .../src/main/java/org/dcm4chee/arc/entity/Location.java | 3 +++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/dcm4chee-arc-delete/src/main/java/org/dcm4chee/arc/delete/impl/DeletionServiceEJB.java b/dcm4chee-arc-delete/src/main/java/org/dcm4chee/arc/delete/impl/DeletionServiceEJB.java index 5b96270cd7..7f0d57cc5f 100644 --- a/dcm4chee-arc-delete/src/main/java/org/dcm4chee/arc/delete/impl/DeletionServiceEJB.java +++ b/dcm4chee-arc-delete/src/main/java/org/dcm4chee/arc/delete/impl/DeletionServiceEJB.java @@ -516,6 +516,14 @@ private boolean hasSeriesWithOtherRejectionState(Study study, RejectionState rej .getSingleResult() > 0; } + public boolean hasInUseFile(String storageID, String storagePath) { + return em.createNamedQuery(Location.COUNT_IN_USE_FILES, Long.class) + .setParameter(1, storageID) + .setParameter(2, Location.Status.OK) + .setParameter(3, storagePath) + .getSingleResult() > 0; + } + private long countStudiesOfPatient(Patient patient) { return em.createNamedQuery(Study.COUNT_STUDIES_OF_PATIENT, Long.class).setParameter(1, patient) .getSingleResult(); diff --git a/dcm4chee-arc-delete/src/main/java/org/dcm4chee/arc/delete/impl/PurgeStorageScheduler.java b/dcm4chee-arc-delete/src/main/java/org/dcm4chee/arc/delete/impl/PurgeStorageScheduler.java index c1289428dc..b9de34f88f 100644 --- a/dcm4chee-arc-delete/src/main/java/org/dcm4chee/arc/delete/impl/PurgeStorageScheduler.java +++ b/dcm4chee-arc-delete/src/main/java/org/dcm4chee/arc/delete/impl/PurgeStorageScheduler.java @@ -502,7 +502,13 @@ private void deleteObjectsFromStorage(ArchiveDeviceExtension arcDev, StorageDesc private void deleteLocation(Storage storage, Location location, AtomicInteger success, AtomicInteger skipped) { try { if (ejb.claimDeleteObject(location)) { - storage.deleteObject(location.getStoragePath()); + if ( !ejb.hasInUseFile(location.getStorageID(), location.getStoragePath()) ) { + // File can actually be removed since not used in any other valid location + storage.deleteObject(location.getStoragePath()); + } + else { + LOG.debug("Location {} deleted from {}, but file is still in use", location, storage); + } ejb.removeLocation(location); LOG.debug("Successfully delete {} from {}", location, storage); success.getAndIncrement(); diff --git a/dcm4chee-arc-entity/src/main/java/org/dcm4chee/arc/entity/Location.java b/dcm4chee-arc-entity/src/main/java/org/dcm4chee/arc/entity/Location.java index 3559653784..439d933027 100644 --- a/dcm4chee-arc-entity/src/main/java/org/dcm4chee/arc/entity/Location.java +++ b/dcm4chee-arc-entity/src/main/java/org/dcm4chee/arc/entity/Location.java @@ -57,6 +57,8 @@ @Index(columnList = "multi_ref") }) @NamedQueries({ + @NamedQuery(name = Location.COUNT_IN_USE_FILES, + query = "select count(l) from Location l where l.storageID=?1 and l.status=?2 and l.storagePath=?3"), @NamedQuery(name = Location.FIND_BY_STORAGE_ID_AND_STATUS, query = "select l from Location l where l.storageID=?1 and l.status=?2"), @NamedQuery(name = Location.FIND_BY_STUDY_PK, @@ -125,6 +127,7 @@ }) public class Location { + public static final String COUNT_IN_USE_FILES = "Location.CountInUseFiles"; public static final String FIND_BY_STORAGE_ID_AND_STATUS = "Location.FindByStorageIDAndStatus"; public static final String FIND_BY_STUDY_PK = "Location.FindByStudyPk"; public static final String FIND_BY_SERIES_PK = "Location.FindBySeriesPk"; From b29fde05505cddd20539641d94faef04cb3186c7 Mon Sep 17 00:00:00 2001 From: rtirabassi Date: Thu, 13 Feb 2020 11:51:31 +0100 Subject: [PATCH 3/3] add: Add multiReference column when creating a LocationQuery.addLocation fix: When previous location is reloaded to check multiReference presence, than the multiReference wasn't tested and a new multiReference ID assigned, this way multi reference chain is broken. --- .../org/dcm4chee/arc/retrieve/impl/LocationQuery.java | 3 ++- .../java/org/dcm4chee/arc/store/impl/StoreServiceEJB.java | 8 +++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/LocationQuery.java b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/LocationQuery.java index 537348060e..0a34da993b 100644 --- a/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/LocationQuery.java +++ b/dcm4chee-arc-retrieve/src/main/java/org/dcm4chee/arc/retrieve/impl/LocationQuery.java @@ -139,6 +139,7 @@ public LocationQuery(EntityManager em, CriteriaBuilder cb, RetrieveContext ctx, locationPath.get(Location_.digest), locationPath.get(Location_.size), locationPath.get(Location_.status), + locationPath.get(Location_.multiReference), series.get(Series_.pk), instance.get(Instance_.pk), instance.get(Instance_.retrieveAETs), @@ -147,7 +148,6 @@ public LocationQuery(EntityManager em, CriteriaBuilder cb, RetrieveContext ctx, instance.get(Instance_.createdTime), instance.get(Instance_.updatedTime), uidMap.get(UIDMap_.pk), - // uidMap.get(UIDMap_.encodedMap), instanceAttrBlob ); } @@ -241,6 +241,7 @@ private void addLocation(InstanceLocations match, Tuple tuple) { .size(tuple.get(locationPath.get(Location_.size))) .status(tuple.get(locationPath.get(Location_.status))) .build(); + location.setMultiReference(tuple.get(locationPath.get(Location_.multiReference))) ; Long uidMapPk = tuple.get(uidMap.get(UIDMap_.pk)); if (uidMapPk != null) { location.setUidMap(ctx.getRetrieveService().getUIDMap(uidMapPk, uidMapCache)); diff --git a/dcm4chee-arc-store/src/main/java/org/dcm4chee/arc/store/impl/StoreServiceEJB.java b/dcm4chee-arc-store/src/main/java/org/dcm4chee/arc/store/impl/StoreServiceEJB.java index 3c03effbdb..b0814fad01 100644 --- a/dcm4chee-arc-store/src/main/java/org/dcm4chee/arc/store/impl/StoreServiceEJB.java +++ b/dcm4chee-arc-store/src/main/java/org/dcm4chee/arc/store/impl/StoreServiceEJB.java @@ -1503,7 +1503,13 @@ private Location copyLocation(StoreSession session, Location prevLocation, Instance instance, Map uidMap, Map uidMapCache) { if (prevLocation.getMultiReference() == null) { prevLocation = em.find(Location.class, prevLocation.getPk()); - prevLocation.setMultiReference(idService.newLocationMultiReference()); + // Since Location members could have been populated by a query result, not containing multiReference value, + // the location itself is now reloaded. + // This means that prevLocation.getMultiReference() can now return a non null value. + // That's why check must be done again before assigning a new value. + if (prevLocation.getMultiReference() == null) { + prevLocation.setMultiReference(idService.newLocationMultiReference()); + } } Location newLocation = new Location(prevLocation); newLocation.setUidMap(createUIDMap(uidMap, prevLocation.getUidMap(), uidMapCache));