You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/02/26 20:58:28 UTC
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4852: NIFI-8212: Refactored StandardExtensionDiscoveringManager to avoid us…
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:
users@infra.apache.org