exceptionfactory commented on a change in pull request #4852:
URL: https://github.com/apache/nifi/pull/4852#discussion_r583902446
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/provenance/ComponentIdentifierLookup.java
##########
@@ -46,14 +48,15 @@ public ComponentIdentifierLookup(final FlowController
flowController) {
@Override
public List<String> getComponentTypes() {
- final Set<Class> procClasses =
flowController.getExtensionManager().getExtensions(Processor.class);
+ final Set<ExtensionDefinition> procDefinitions =
flowController.getExtensionManager().getExtensions(Processor.class);
- final List<String> componentTypes = new ArrayList<>(procClasses.size()
+ 2);
+ final List<String> componentTypes = new
ArrayList<>(procDefinitions.size() + 2);
componentTypes.add(ProvenanceEventRecord.REMOTE_INPUT_PORT_TYPE);
componentTypes.add(ProvenanceEventRecord.REMOTE_OUTPUT_PORT_TYPE);
- procClasses.stream()
- .map(Class::getSimpleName)
+ procDefinitions.stream()
+ .map(ExtensionDefinition::getImplementationClassName)
+ .map(className -> className.contains(".") ?
StringUtils.substringAfterLast(className, ".") : className)
Review comment:
The `StandardStatelessEngine` uses the same approach attempting to
derive the simple class name, does it make sense to add a method on
`ExtensionDefinition` to return the simple class name?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-nar-utils/src/main/java/org/apache/nifi/nar/NarUnpacker.java
##########
@@ -288,17 +280,20 @@ private static void unpackBundleDocs(final File
docsDirectory, final ExtensionMa
*
* @param nar the nar to unpack
* @param baseWorkingDirectory the directory to unpack to
+ * @param verifyHash if the NAR has already been unpacked, indicates
whether or not the hash should be verified. If this value is true,
+ * and the NAR's hash does not match the hash written to the unpacked
directory, the working directory will be deleted and the NAR will be
+ * unpacked again. If false, the NAR will not be unpacked again and its
hash will not be checked.
* @return the directory to the unpacked NAR
* @throws IOException if unable to explode nar
*/
- public static File unpackNar(final File nar, final File
baseWorkingDirectory) throws IOException {
+ public static File unpackNar(final File nar, final File
baseWorkingDirectory, final boolean verifyHash) throws IOException {
final File narWorkingDirectory = new File(baseWorkingDirectory,
nar.getName() + "-unpacked");
// if the working directory doesn't exist, unpack the nar
if (!narWorkingDirectory.exists()) {
unpack(nar, narWorkingDirectory, FileDigestUtils.getDigest(nar));
- } else {
- // the working directory does exist. Run digest against the nar
+ } else if (verifyHash) {
+ // the working directory does exist. Run MD5 sum against the nar
Review comment:
NIFI-8132 changed the hash to SHA-256, so the previous comment used the
general term `digest` as opposed to referencing the particular algorithm. This
could be changed to say `SHA-256`, or reverted.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-nar-utils/src/main/java/org/apache/nifi/nar/StandardExtensionDiscoveringManager.java
##########
@@ -145,74 +152,103 @@ public void discoverExtensions(final Set<Bundle>
narBundles) {
*
* @param bundle from which to load extensions
*/
- @SuppressWarnings("unchecked")
private void loadExtensions(final Bundle bundle) {
- for (final Map.Entry<Class, Set<Class>> entry :
definitionMap.entrySet()) {
- final boolean isControllerService =
ControllerService.class.equals(entry.getKey());
- final boolean isProcessor = Processor.class.equals(entry.getKey());
- final boolean isReportingTask =
ReportingTask.class.equals(entry.getKey());
-
- final ServiceLoader<?> serviceLoader =
ServiceLoader.load(entry.getKey(), bundle.getClassLoader());
- for (final Object o : serviceLoader) {
- try {
- loadExtension(o, entry.getKey(), bundle);
- } catch (Exception e) {
- logger.warn("Failed to register extension {} due to: {}" ,
new Object[]{o.getClass().getCanonicalName(), e.getMessage()});
- if (logger.isDebugEnabled()) {
- logger.debug("", e);
+ for (final Class extensionType : definitionMap.keySet()) {
+ final String serviceType = extensionType.getName();
+
+ try {
+ final Set<URL> serviceResourceUrls =
getServiceFileURLs(bundle, extensionType);
+ logger.debug("Bundle {} has the following Services File URLs
for {}: {}", bundle, serviceType, serviceResourceUrls);
+
+ for (final URL serviceResourceUrl : serviceResourceUrls) {
+ final Set<String> implementationClassNames =
getServiceFileImplementationClassNames(serviceResourceUrl);
+ logger.debug("Bundle {} defines {} implementations of
interface {}", bundle, implementationClassNames.size(), serviceType);
+
+ for (final String implementationClassName :
implementationClassNames) {
+ try {
+ loadExtension(implementationClassName,
extensionType, bundle);
+ logger.debug("Successfully loaded {} {} from {}",
extensionType.getSimpleName(), implementationClassName, bundle);
+ } catch (final Exception e) {
+ logger.error("Failed to register {} of type {} in
bundle {}" , extensionType.getSimpleName(), implementationClassName, bundle, e);
+ }
}
}
+ } catch (final IOException e) {
+ throw new RuntimeException("Failed to get resources of type "
+ serviceType + " from bundle " + bundle);
}
-
- classLoaderBundleLookup.put(bundle.getClassLoader(), bundle);
}
+
+ classLoaderBundleLookup.put(bundle.getClassLoader(), bundle);
}
- protected void loadExtension(final Object extension, final Class<?>
extensionType, final Bundle bundle) {
- final boolean isControllerService =
ControllerService.class.equals(extensionType);
- final boolean isProcessor = Processor.class.equals(extensionType);
- final boolean isReportingTask =
ReportingTask.class.equals(extensionType);
+ private Set<String> getServiceFileImplementationClassNames(final URL
serviceFileUrl) throws IOException {
+ final Set<String> implementationClassNames = new HashSet<>();
- // create a cache of temp ConfigurableComponent instances, the
initialize here has to happen before the checks below
- if ((isControllerService || isProcessor || isReportingTask) &&
extension instanceof ConfigurableComponent) {
- final ConfigurableComponent configurableComponent =
(ConfigurableComponent) extension;
- initializeTempComponent(configurableComponent);
+ try (final InputStream in = serviceFileUrl.openStream();
+ final Reader inputStreamReader = new InputStreamReader(in);
+ final BufferedReader reader = new
BufferedReader(inputStreamReader)) {
- final String cacheKey =
getClassBundleKey(extension.getClass().getCanonicalName(),
bundle.getBundleDetails().getCoordinate());
- tempComponentLookup.put(cacheKey, configurableComponent);
- }
+ String line;
+ while ((line = reader.readLine()) != null) {
+ // Remove anything after the #
+ final int index = line.indexOf("#");
+ if (index >= 0) {
+ line = line.substring(0, index);
+ }
- // only consider extensions discovered directly in this bundle
- boolean registerExtension =
bundle.getClassLoader().equals(extension.getClass().getClassLoader());
+ // Ignore empty line
+ line = line.trim();
+ if (line.isEmpty()) {
+ continue;
+ }
- if (registerExtension) {
- final Class<?> extensionClass = extension.getClass();
- if (isControllerService &&
!checkControllerServiceEligibility(extensionClass)) {
- registerExtension = false;
- logger.error(String.format(
- "Skipping Controller Service %s because it is bundled with
its supporting APIs and requires instance class loading.",
extensionClass.getName()));
+ implementationClassNames.add(line);
Review comment:
The JDK `ServiceLoader` includes a check of the first character in a
line that calls `Character.isJavaIdentifierStart()`, is it worth introducing
such as check at this point? The result of the check could throw an exception
indicating an invalid line in the service file, as opposed to throwing an error
somewhere higher in the chain when attempting to instantiate an class from an
invalid string.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-documentation/src/main/java/org/apache/nifi/documentation/DocGenerator.java
##########
@@ -69,25 +70,34 @@ public static void generate(final NiFiProperties
properties, final ExtensionMana
/**
* Documents a type of configurable component.
*
- * @param extensionClasses types of a configurable component
+ * @param extensionDefinitions definitions of the extensions to document
* @param explodedNiFiDocsDir base directory of component documentation
*/
- public static void documentConfigurableComponent(final Set<Class>
extensionClasses, final File explodedNiFiDocsDir, final ExtensionManager
extensionManager) {
- for (final Class<?> extensionClass : extensionClasses) {
- if (ConfigurableComponent.class.isAssignableFrom(extensionClass)) {
- final String extensionClassName =
extensionClass.getCanonicalName();
-
- final Bundle bundle =
extensionManager.getBundle(extensionClass.getClassLoader());
- if (bundle == null) {
- logger.warn("No coordinate found for {}, skipping...", new
Object[] {extensionClassName});
- continue;
- }
- final BundleCoordinate coordinate =
bundle.getBundleDetails().getCoordinate();
+ public static void documentConfigurableComponent(final
Set<ExtensionDefinition> extensionDefinitions, final File explodedNiFiDocsDir,
final ExtensionManager extensionManager) {
+ for (final ExtensionDefinition extensionDefinition :
extensionDefinitions) {
+ final Bundle bundle = extensionDefinition.getBundle();
+ if (bundle == null) {
+ logger.warn("Cannot document extension {} because it has no
bundle associated with it", extensionDefinition);
+ continue;
+ }
+
+ final BundleCoordinate coordinate =
bundle.getBundleDetails().getCoordinate();
+
+ final String extensionClassName =
extensionDefinition.getImplementationClassName();
+ final String path = coordinate.getGroup() + "/" +
coordinate.getId() + "/" + coordinate.getVersion() + "/" + extensionClassName;
+ final File componentDirectory = new File(explodedNiFiDocsDir,
path);
+ final File indexHtml = new File(componentDirectory, "index.html");
Review comment:
It looks like the `index.html` filename string is referenced multiple
times in this class. Is it worth creating a static variable to define it once
and reuse it here as well as in the `document()` method?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-stateless-bundle/nifi-stateless-engine/src/main/java/org/apache/nifi/extensions/FileSystemExtensionRepository.java
##########
@@ -135,6 +135,8 @@ public BundleAvailability getBundleAvailability(final
BundleCoordinate bundleCoo
}
final Set<Bundle> loadedBundles = narLoadResult.getLoadedBundles();
+
+// extensionManager.registerBundles(loadedBundles);
Review comment:
Should this line remain commented, or be removed?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-stateless-bundle/nifi-stateless-engine/src/main/java/org/apache/nifi/stateless/flow/StandardStatelessDataflowFactory.java
##########
@@ -148,7 +149,19 @@ public StatelessDataflow createDataflow(final
StatelessEngineConfiguration engin
narClassLoaders, extensionClients);
final VariableRegistry variableRegistry =
VariableRegistry.EMPTY_REGISTRY;
- final PropertyEncryptor encryptor =
getPropertyEncryptor(engineConfiguration.getSensitivePropsKey());
+ final Supplier<PropertyEncryptor> encryptorFactory = new
Supplier<PropertyEncryptor>() {
+ private PropertyEncryptor created = null;
+
+ @Override
+ public synchronized PropertyEncryptor get() {
Review comment:
Is it worth considering a more optimized locking strategy that
method-level synchronization since this may get called multiple times? Some
options include checking the `PropertyEncryptor` instance for null and then
synchronizing around that object, or leveraging Apache Commons
`AtomicInitializer`, or implementing something along the lines of that class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
