[GitHub] [kafka] gharris1727 commented on pull request #13467: KAFKA-14863: Hide plugins with invalid constructors during plugin discovery

["gharris1727 (via GitHub)" <gi...@apache.org>]

gharris1727 commented on PR #13467:
URL: https://github.com/apache/kafka/pull/13467#issuecomment-1568821952

   > Can you describe the impact on users with this change if a plugin with no valid constructor is present on the worker?
   
   Suppose you had a Connector without a valid constructor.
   
   1. Before KAFKA-14649, your worker would crash on plugin path scanning with an opaque error.
   2. After KAFKA-14649, the worker logs an informative error, hides the plugin with a 404 in the REST API and throws ClassNotFoundExceptions for existing connector instances.
   
   Suppose you had a Converter without a valid constructor.
   
   1. Before KAFKA-14863 (this change), the worker would not crash or log an error. The plugin would be visible in the REST API, and would throw an opaque NoSuchMethodException if used in existing connectors.
   2. After KAFKA-14863 (this change), the worker logs an informative error, hides the plugin with a 404 in the REST API, and throws ClassNotFoundExceptions for existing connectors that use this converter.
   
   > One alternative I'm considering is if we try to make more plugins visible, even if they cannot be used, if that ultimately makes it easier for users to diagnose issues with them (a stack trace from a REST request is often more informative than a message in the worker's logs).
   
   While this is reducing the diagnostic information present in the REST API (ClassNotFoundException is less specific than NoSuchMethodException) it is making the REST API more consistent (a class that throws ClassNotFoundException in validate is also a 404 on the plugins endpoint). I don't think it's inappropriate to have users diagnose startup packaging problems with error logs, especially when the REST API information before was rather opaque. 
   
   The trouble with keeping the current reflective behavior (showing plugins with bad constructors) is that it is difficult to replicate with the ServiceLoader. From the ServiceLoader interface, we do not see the implementation's class name until after the static initializer and constructor has completed without error. If we wished to collect the class name information for badly packaged plugins (instead of just hiding them) we'd be required to fork the ServiceLoader or depend on error messages to extract class names.
   
   > Also, can you shed some light on the implications this has for [KIP-898](https://cwiki.apache.org/confluence/display/KAFKA/KIP-898%3A+Modernize+Connect+plugin+discovery)? For example, will the behavior of the migration script change?
   
   1. Without KAFKA-14863 (this change), the migration script produces an "incomplete" migration. Even if the migration script generated an appropriate ServiceLoader manifest, the plugin would appear as if no migration took place. Assertions that all plugins were migrated would fail.
   2. With KAFKA-14863 (this change), the migration script hides and does not migrate plugins which are missing a valid constructor. After a migration is complete, all plugins which are present on the REST API before migration are present after, so assertions that all plugins were migrated would pass.
   
   This is the core motivation of this PR: the reflective scanning and ServiceLoader scanning error handling should have parity, so that plugins which are not usable with the ServiceLoader aren't migrated, and the migration script still includes all of the plugins which are visible in the REST API. If we drop one of these two properties, then the migration looks incomplete to the user.
   
   I examined some plugin implementations out in the wild, and observed that plugins which are packaged and don't have a valid constructor often appear in the REST API accidentally. For example, many packaged SMTs have concrete base classes without a valid constructor, intending instead to be referenced via their `$Key` or $`Value` subclasses. This change is de-cluttering the REST API of all of these unintentionally-concrete classes, each of which would cause poor migration behavior.


-- 
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.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org