Robin Shen [03/Dec/16 01:28 AM] Artifact cleanup only works at system maintenance time. Can you please check if you've changed maintenance schedule recently in system setting?

AlSt [05/Dec/16 08:16 AM] No, system maintenance time is unchanged. It's set to "0 0 0,6,12,18 * * ?" and it successfully removes all builds older than 7 days, but it does not clean the artifacts!

AlSt [05/Dec/16 03:21 PM] Actually it cleans something, but the wrong stuff. Also the latest build was cleaned. I took a look at the cleanup task and there is one thing that might be better. Remove the last 5 buildIds before sending them to the build manager and doing the batch stuff. I do no know what else is the issue. And if I can suggest one thing: @SessionAware @Override public void doBatch(Configuration configuration, List<Long> buildIds, BuildRunnable runnable, int batchSize) { Session session = getSession(); int count = buildIds.size(); for (int i=0; i<count/batchSize; i++) { doFor(configuration, buildIds, runnable, i*batchSize, Math.min((i+1)*batchSize, count)-1); // clear session to free memory getSession().clear(); } } private void doFor(Configuration configuration, List<Long> buildIds, BuildRunnable runnable, int start, int end) { Long fromId = buildIds.get(start); Long toId = buildIds.get(end); logger.info("Performing task for builds ranging from {} to {}...", fromId, toId); Query query = getSession().createQuery("from Build where configuration=:configuration and id>=:fromId and id<=:toId"); query.setParameter("configuration", configuration); query.setParameter("fromId", fromId); query.setParameter("toId", toId); for (Build build: (List<Build>)query.list()) { runnable.doFor(build); } } Or you might even pass a sublist with a "id in (:buildIds)" as you fetch the build ids first and use only some of them.

AlSt [05/Dec/16 03:25 PM] Please be aware that this code is untested. And I just saw that the count/batchSize in the for loop needs a Math.ceil(count/batchSize) to work properly. And as I said. You can get rid of the AtomicInteger in the cleanuptask when you remove the last 5 build ids (the number of builds where the artifacts should be kept) before passing them to the doBatch method.

AlSt [05/Dec/16 03:30 PM] And one final comment: It really removed all artifacts except 5, but not the latest 5. I think I know the problem. in the doFor the query does not contain sorting in any way. So the builds are returned in any order. So my remove beforehand would also solve this. So I think to really fix this: * Remove the latest 5 build ids beforehand and get rid of the atomicinteger which is not needed anymore. * Use a sublist and a HQL "in" * Add ordering to the HQL query in doFor This should avoid further problems.

AlSt [05/Dec/16 03:33 PM] Ok another comment. Sorry for spamming. There are so many things that come to my mind after every comment. Also when you use HQL with IN it should be faster because it does not need to do a join because you can remove the configuration parameter. It will only touch the builds with the given ids anyway.

Robin Shen [06/Dec/16 03:38 AM] Thanks for the comment. The unsorted iteration in doFor() does impose issues when working together with increasing AtomicInteger. If we exclude latest N builds before calling doBatch, things should be fine. Also not using Math.ceil here should be fine, as we just want the integer division, and there is a last statement to complement the logic: if (count%batchSize != 0) { doFor(configuration, buildIds, runnable, count/batchSize*batchSize, count%batchSize); getSession().clear(); } It is more complicated than your approach, but it is equivalent I think. As to hibernate "IN" statement, we do not use that as it can easily cause database complaining about "too long sql". What do you think?

AlSt [06/Dec/16 08:08 AM] You're right. The code does basically the same. The Math.ceil((float)count/batchSize) is only for the suggested code, sorry for the confusion there. It would then only need the loop and no special handling afterwards. But yes it should work in both ways. The HQL IN should not cause "too long sql" when the batch size is not too high. But it would prevent touching builds which were not fetched before and is faster because no JOIN is needed. The main issue is of course the sorting. So I think at least sorting should be added (to avoid the same problem at another point) and the latest X build IDs should be excluded first to get rid of the AtomicInteger.

Robin Shen [06/Dec/16 09:50 AM] 6.1.35 fixed this. Sorting is not necessary if latest X builds are excluded. As non-sorting also meets purpose of the batch processing: process all builds one by one