exceptionfactory commented on a change in pull request #4852:
URL: https://github.com/apache/nifi/pull/4852#discussion_r585752120
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-nar-utils/src/main/java/org/apache/nifi/nar/StandardExtensionDiscoveringManager.java
##########
@@ -128,7 +131,11 @@ public void discoverExtensions(final Set<Bundle>
narBundles) {
// so that static initialization techniques that depend on the
context class loader will work properly
final ClassLoader ncl = bundle.getClassLoader();
Thread.currentThread().setContextClassLoader(ncl);
+
+ final long loadStart = System.currentTimeMillis();
loadExtensions(bundle);
+ final long loadMillis = System.currentTimeMillis() - loadStart;
+ logger.info("Loaded extensions for {} in {} millis",
bundle.getBundleDetails(), loadMillis);
Review comment:
Should this timing information be logged at the info level, that can
produce a number of logs given that it runs for each bundle.
##########
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:
Thanks for the reply, that sounds reasonable.
##########
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:
That makes sense, given it is only in two places, not a real concern
right now.
##########
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:
Thanks for the reply. The Character is check appears to be just a
sanity check in ServiceLoader. Invalid class names will throw other errors, so
it is really just a matter of where NiFi catches the problem. Leaving out the
check is fine if it is reasonable to wait until the class gets loaded to
indicate what is most likely a typo.
----------------------------------------------------------------
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]
