implements lilt connector

[tgallant]

Changes

• rebrands bootstrap connector to lilt connector
• adds LiltApiClient
• implements lilt translation service

[tingley]

Looks pretty good! Mostly stylistic issues (some of the indentation is wonky). I think a some of the exception handling needs to be looked at, I think it can produce weird crashes or bad behaviors -- although I don't know how AEM expects you to handle some of these cases.

Also, in a lot of java apps, running things at debug or trace log levels produces an incredible blizzard of output, so if any of the things you are logging at those levels are at all important, it might be worth promoting them to INFO.

[tingley]

It looks like the bootstrap code is using a different indentation than your editor is.. you should probably resolve this one way or the other

[tingley]

Minor style thing - you can infer the type parameters in the constructor:

Suggested change

	Map<String, Object> param = new HashMap<String, Object>();
	Map<String, Object> param = new HashMap<>();

[tingley]

It looks like this results in the translation service still being created, but with liltApiClient being null -- is that the right failure mode? It looks like it might cause crashes later on.

[tingley]

If you want to disable the pseudotranslation option, maybe better to just remove this conditional entirely? (Or leave it intact and uncomment..)

[tingley]

Similarly, this doesn't seem like the right error handling. I'm not sure how the bootstrap framework is set up to handle failure in general. In the core we would often have a function like this return Optional<String> instead. Re-throwing the exception after logging it might also work, depending on where that exception is caught.

[tingley]

There's a stream idiom you could use to write this more concisely:

int imported = files.stream().filter(f -> f.labels.contains("status=IMPORTED")).count();

But since you're computing two values, I don't think there's a good way to use that without iterating over the loop twice. So what you have is fine.

[tingley]

I'm not sure there's actually bug here, but there's the potential for the class of bug where the input stream is never closed and you leak either an open socket or an open file handle if an exception is thrown from the wrong place in this method (or another method that it calls), before the stream contents are consumed by uploadFile() (which probably closes the stream).

The old-fashioned way to deal with this is to wrap everything below this line in a try/finally block:

InputStream inputStream;
try {
  // do stuff
}
finally {
  if (inputStream != null) {
   inputStream.close();
}

These days you should just use the "try with resources" feature, which makes the finally implicit. However, do to this you will want to split the various things that can produce the inputStream out into a separate method. You can also fold in your existing try/catch block here:

// Define getStreamFromTranslationObject elsewhere.  This syntactic structure defines an implicit finally
// that will automatically close the InputStream (or anything else declared this way that implements
// the AutoCloseable interface.
try (InputStream inputStream = getStreamFromTranslationObject(translationObject, exportFormat) {
  // ....
  String labels = String.format("%s,status=READY", strTranslationJobID);
  liltApiClient.uploadFile(objectPath, labels, inputStream);
}
catch (Exception e) {
  log.warn("error during uploadTranslationObject", e);
}

[tingley]

log4j slf4j has a special method signature when there is only one object parameter that is an exception/throwable -- in this case, it will automatically log the full stack track. In this case, you don't want to interpolate it with {}, which will either only log the error message of the exception, or maybe just log a raw {}. (In this specific case, I'm not sure.)

So better to use:

Suggested change

	log.warn("error during getTranslatedObject {}", e);
	log.warn("error during getTranslatedObject", e);

There are a bunch of instances like this, I didn't flag them all.

[tingley]

Similarly, combine this line and the next into:

try (ZipInputStream zipInputStream = translationObject.getTranslationObjectPreview()) {

// ...

[tingley]

Not a big deal, but you can declare this as a class member (even a static one) and reuse it. It's threadsafe once constructed.