Batch 4 of changes from of review comments - done for now

This commit is contained in:
alandavis
2022-09-08 18:38:42 +01:00
parent 4a554d374e
commit 59f082d9d4
9 changed files with 48 additions and 32 deletions

View File

@@ -233,7 +233,7 @@ public class TransformController
public String ready(HttpServletRequest request) public String ready(HttpServletRequest request)
{ {
// An alternative without transforms might be: ((TransformRegistry)transformRegistry).isReadyForTransformRequests(); // An alternative without transforms might be: ((TransformRegistry)transformRegistry).isReadyForTransformRequests();
return getProbeTransform().doTransformOrNothing(request, false, transformHandler); return getProbeTransform().doTransformOrNothing(false, transformHandler);
} }
/** /**
@@ -243,7 +243,7 @@ public class TransformController
@ResponseBody @ResponseBody
public String live(HttpServletRequest request) public String live(HttpServletRequest request)
{ {
return getProbeTransform().doTransformOrNothing(request, true, transformHandler); return getProbeTransform().doTransformOrNothing(true, transformHandler);
} }
public ProbeTransform getProbeTransform() public ProbeTransform getProbeTransform()

View File

@@ -32,7 +32,6 @@ import org.alfresco.transform.common.TransformException;
import org.slf4j.Logger; import org.slf4j.Logger;
import org.slf4j.LoggerFactory; import org.slf4j.LoggerFactory;
import javax.servlet.http.HttpServletRequest;
import java.io.File; import java.io.File;
import java.io.IOException; import java.io.IOException;
import java.io.InputStream; import java.io.InputStream;
@@ -121,7 +120,7 @@ public class ProbeTransform
this.sourceFilename = sourceFilename; this.sourceFilename = sourceFilename;
this.sourceMimetype = sourceMimetype; this.sourceMimetype = sourceMimetype;
this.targetMimetype = targetMimetype; this.targetMimetype = targetMimetype;
this.transformOptions = new HashMap(transformOptions); this.transformOptions = new HashMap<>(transformOptions);
minExpectedLength = Math.max(0, expectedLength - plusOrMinus); minExpectedLength = Math.max(0, expectedLength - plusOrMinus);
maxExpectedLength = expectedLength + plusOrMinus; maxExpectedLength = expectedLength + plusOrMinus;
@@ -168,7 +167,7 @@ public class ProbeTransform
} }
// We don't want to be doing test transforms every few seconds, but do want frequent live probes. // We don't want to be doing test transforms every few seconds, but do want frequent live probes.
public String doTransformOrNothing(HttpServletRequest request, boolean isLiveProbe, TransformHandler transformHandler) public String doTransformOrNothing(boolean isLiveProbe, TransformHandler transformHandler)
{ {
// If not initialised OR it is a live probe and we are scheduled to to do a test transform. // If not initialised OR it is a live probe and we are scheduled to to do a test transform.
probeCount++; probeCount++;
@@ -215,7 +214,7 @@ public class ProbeTransform
transformHandler.handleProbeRequest(sourceMimetype, targetMimetype, transformOptions, sourceFile, targetFile, this); transformHandler.handleProbeRequest(sourceMimetype, targetMimetype, transformOptions, sourceFile, targetFile, this);
long time = System.currentTimeMillis() - start; long time = System.currentTimeMillis() - start;
String message = "Transform " + time + "ms"; String message = "Transform " + time + "ms";
checkTargetFile(targetFile, isLiveProbe, message); checkTargetFile(targetFile, isLiveProbe);
recordTransformTime(time); recordTransformTime(time);
calculateMaxTime(time, isLiveProbe); calculateMaxTime(time, isLiveProbe);
@@ -312,7 +311,7 @@ public class ProbeTransform
} }
} }
private void checkTargetFile(File targetFile, boolean isLiveProbe, String message) private void checkTargetFile(File targetFile, boolean isLiveProbe)
{ {
String probeMessage = getProbeMessage(isLiveProbe); String probeMessage = getProbeMessage(isLiveProbe);
if (!targetFile.exists() || !targetFile.isFile()) if (!targetFile.exists() || !targetFile.isFile())

View File

@@ -45,19 +45,19 @@ public class CustomTransformers
private static final Logger logger = LoggerFactory.getLogger(CustomTransformers.class); private static final Logger logger = LoggerFactory.getLogger(CustomTransformers.class);
@Autowired(required = false) @Autowired(required = false)
private List<CustomTransformer> customTransformers; private List<CustomTransformer> customTransformerList;
private final Map<String, CustomTransformer> customTransformersByName = new HashMap<>(); private final Map<String, CustomTransformer> customTransformersByName = new HashMap<>();
@PostConstruct @PostConstruct
private void initCustomTransformersByName() private void initCustomTransformersByName()
{ {
if (customTransformers != null) if (customTransformerList != null)
{ {
customTransformers.forEach(customTransformer -> customTransformerList.forEach(customTransformer ->
customTransformersByName.put(customTransformer.getTransformerName(), customTransformer)); customTransformersByName.put(customTransformer.getTransformerName(), customTransformer));
List<String> nonNullTransformerNames = customTransformers.stream() List<String> nonNullTransformerNames = customTransformerList.stream()
.map(CustomTransformer::getTransformerName) .map(CustomTransformer::getTransformerName)
.filter(Objects::nonNull) .filter(Objects::nonNull)
.collect(Collectors.toList()); .collect(Collectors.toList());
@@ -87,6 +87,6 @@ public class CustomTransformers
public List<CustomTransformer> toList() public List<CustomTransformer> toList()
{ {
return customTransformers; return customTransformerList;
} }
} }

View File

@@ -88,7 +88,7 @@ public class TransformRegistry extends AbstractTransformRegistry
private boolean isTRouter; private boolean isTRouter;
// Not autowired - avoids a circular reference in the router - initialised on startup event // Not autowired - avoids a circular reference in the router - initialised on startup event
private List<CustomTransformer> customTransformers; private List<CustomTransformer> customTransformerList;
private int previousLogMessageHashCode; private int previousLogMessageHashCode;
@@ -127,6 +127,11 @@ public class TransformRegistry extends AbstractTransformRegistry
{ {
this.transformerByNameMap = transformerByNameMap; this.transformerByNameMap = transformerByNameMap;
} }
public int getTransformCount()
{
return transformCount;
}
} }
private Data data = new Data(); private Data data = new Data();
@@ -152,7 +157,7 @@ public class TransformRegistry extends AbstractTransformRegistry
backoff = @Backoff(delayExpression = "#{${transform.engine.config.retry.timeout} * 1000}")) backoff = @Backoff(delayExpression = "#{${transform.engine.config.retry.timeout} * 1000}"))
void initRegistryOnAppStartup(final ContextRefreshedEvent event) void initRegistryOnAppStartup(final ContextRefreshedEvent event)
{ {
customTransformers = event.getApplicationContext().getBean(CustomTransformers.class).toList(); customTransformerList = event.getApplicationContext().getBean(CustomTransformers.class).toList();
retrieveConfig(); retrieveConfig();
} }
@@ -196,17 +201,16 @@ public class TransformRegistry extends AbstractTransformRegistry
Map<String, Origin<Transformer>> transformerByNameMap = combinedTransformConfig.getTransformerByNameMap(); Map<String, Origin<Transformer>> transformerByNameMap = combinedTransformConfig.getTransformerByNameMap();
concurrentUpdate(combinedTransformConfig, uncombinedTransformConfig, transformConfig, transformerByNameMap); concurrentUpdate(combinedTransformConfig, uncombinedTransformConfig, transformConfig, transformerByNameMap);
logTransformers(uncombinedTransformConfig, combinedTransformConfig, transformerByNameMap); logTransformers(uncombinedTransformConfig, transformerByNameMap);
} }
private void logTransformers(TransformConfig uncombinedTransformConfig, CombinedTransformConfig combinedTransformConfig, private void logTransformers(TransformConfig uncombinedTransformConfig, Map<String, Origin<Transformer>> transformerByNameMap)
Map<String, Origin<Transformer>> transformerByNameMap)
{ {
if (logger.isInfoEnabled()) if (logger.isInfoEnabled())
{ {
Set<String> customTransformerNames = new HashSet(customTransformers == null Set<String> customTransformerNames = new HashSet<>(customTransformerList == null
? Collections.emptySet() ? Collections.emptySet()
: customTransformers.stream().map(CustomTransformer::getTransformerName).collect(Collectors.toSet())); : customTransformerList.stream().map(CustomTransformer::getTransformerName).collect(Collectors.toSet()));
List<String> nonNullTransformerNames = uncombinedTransformConfig.getTransformers().stream() List<String> nonNullTransformerNames = uncombinedTransformConfig.getTransformers().stream()
.map(Transformer::getTransformerName) .map(Transformer::getTransformerName)
.filter(Objects::nonNull) .filter(Objects::nonNull)
@@ -214,7 +218,7 @@ public class TransformRegistry extends AbstractTransformRegistry
ArrayList<String> logMessages = new ArrayList<>(); ArrayList<String> logMessages = new ArrayList<>();
if (!nonNullTransformerNames.isEmpty()) if (!nonNullTransformerNames.isEmpty())
{ {
logMessages.add("Transformers (" + nonNullTransformerNames.size() + "):"); logMessages.add("Transformers (" + nonNullTransformerNames.size() + ") Transforms (" + getData().getTransformCount()+ "):");
nonNullTransformerNames nonNullTransformerNames
.stream() .stream()
.sorted(String.CASE_INSENSITIVE_ORDER) .sorted(String.CASE_INSENSITIVE_ORDER)

View File

@@ -85,13 +85,10 @@ import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR;
@Component @Component
public class TransformHandler public class TransformHandler
{ {
private static final Logger logger = LoggerFactory.getLogger(TransformHandler.class); private static final Logger logger = LoggerFactory.getLogger(TransformHandler.class);
private static final String FAILED_WRITING_TO_SFS = "Failed writing to SFS"; private static final String FAILED_WRITING_TO_SFS = "Failed writing to SFS";
@Autowired(required = false)
private List<TransformEngine> transformEngines;
@Autowired(required = false) @Autowired(required = false)
private CustomTransformers customTransformers; private CustomTransformers customTransformers;
@Autowired @Autowired
@@ -267,7 +264,7 @@ public class TransformHandler
reply.setErrorDetails(messageWithCause("Transform failed", e)); reply.setErrorDetails(messageWithCause("Transform failed", e));
transformerDebug.logFailure(reply); transformerDebug.logFailure(reply);
logger.trace("Transform failed. Sending " + reply, e); logger.trace("Transform failed. Sending {}", reply, e);
transformReplySender.send(replyToQueue, reply); transformReplySender.send(replyToQueue, reply);
} }

View File

@@ -202,7 +202,7 @@ public class TransformManagerImpl implements TransformManager
{ {
if (sourceFile != null && !sourceFile.delete()) if (sourceFile != null && !sourceFile.delete())
{ {
logger.error("Failed to delete temporary source file "+sourceFile.getPath()); logger.error("Failed to delete temporary source file {}", sourceFile.getPath());
} }
outputStreamLengthRecorder = null; outputStreamLengthRecorder = null;
sourceFile = null; sourceFile = null;
@@ -214,7 +214,7 @@ public class TransformManagerImpl implements TransformManager
{ {
if (!keepTargetFile && targetFile != null && !targetFile.delete()) if (!keepTargetFile && targetFile != null && !targetFile.delete())
{ {
logger.error("Failed to delete temporary target file "+targetFile.getPath()); logger.error("Failed to delete temporary target file {}", targetFile.getPath());
} }
targetFile = null; targetFile = null;
createTargetFileCalled = false; createTargetFileCalled = false;

View File

@@ -61,6 +61,8 @@ import java.nio.file.Files;
import java.util.HashMap; import java.util.HashMap;
import java.util.Map; import java.util.Map;
import java.util.UUID; import java.util.UUID;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicLong;
import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
import static org.alfresco.transform.common.RequestParamMap.DIRECT_ACCESS_URL; import static org.alfresco.transform.common.RequestParamMap.DIRECT_ACCESS_URL;
@@ -284,6 +286,20 @@ public abstract class AbstractBaseTest
.build(); .build();
} }
public static void resetProbeForTesting(ProbeTransform probe)
{
ReflectionTestUtils.setField(probe, "probeCount", 0);
ReflectionTestUtils.setField(probe, "transCount", 0);
ReflectionTestUtils.setField(probe, "normalTime", 0);
ReflectionTestUtils.setField(probe, "maxTime", Long.MAX_VALUE);
ReflectionTestUtils.setField(probe, "nextTransformTime", 0);
((AtomicBoolean)ReflectionTestUtils.getField(probe, "initialised")).set(false);
((AtomicBoolean)ReflectionTestUtils.getField(probe, "readySent")).set(false);
((AtomicLong)ReflectionTestUtils.getField(probe, "transformCount")).set(0);
((AtomicBoolean)ReflectionTestUtils.getField(probe, "die")).set(false);
}
@Test @Test
public void simpleTransformTest() throws Exception public void simpleTransformTest() throws Exception
{ {
@@ -325,7 +341,7 @@ public abstract class AbstractBaseTest
public void calculateMaxTime() throws Exception public void calculateMaxTime() throws Exception
{ {
ProbeTransform probeTransform = controller.getProbeTransform(); ProbeTransform probeTransform = controller.getProbeTransform();
probeTransform.resetForTesting(); resetProbeForTesting(probeTransform);
probeTransform.setLivenessPercent(110); probeTransform.setLivenessPercent(110);
long[][] values = new long[][]{ long[][] values = new long[][]{

View File

@@ -131,7 +131,7 @@ public class TransformControllerTest
static void resetProbeForTesting(TransformController transformController) static void resetProbeForTesting(TransformController transformController)
{ {
transformController.getProbeTransform().resetForTesting(); AbstractBaseTest.resetProbeForTesting(transformController.getProbeTransform());
} }
@Test @Test

View File

@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="UTF-8"?> j<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion> <modelVersion>4.0.0</modelVersion>
<name>- Model</name> <name>- Model</name>