New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-7056: Moved Connect’s new numeric converters to runtime (KIP-305) #5222
Conversation
KIP-305 added numeric converters to Connect, but these were added in Connect’s API module in the same package as the `StringConverter`. This commit moves them into the Runtime module and into the `converters` package where the `ByteArrayConverter` already lives. These numeric converters have not yet been included in a release, and so they can be moved without concern. All of Connect’s converters must be referenced in worker / connector configurations and are therefore part of the API, but otherwise do not need to be in the “api” module as they do not need to be instantiated or directly used by extensions. This change makes them more similar to and aligned with the `ByteArrayConverter`. It also gives us the opportunity to move them into the “api” module in the future (keeping the same package name), should we ever want or need to do so. However, if we were to start out with them in the “api” module, we would never be able to move them out into the “runtime” module, even if we kept the same package name. Therefore, moving them to “runtime” now gives us a bit more flexibility.
PR build failure on JDK 8 is unrelated: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is trivial. Would be could to happen before the initial release of these converters.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks. Merging to trunk and 2.0
KIP-305 added numeric converters to Connect, but these were added in Connect’s API module in the same package as the `StringConverter`. This commit moves them into the Runtime module and into the `converters` package where the `ByteArrayConverter` already lives. These numeric converters have not yet been included in a release, and so they can be moved without concern. All of Connect’s converters must be referenced in worker / connector configurations and are therefore part of the API, but otherwise do not need to be in the “api” module as they do not need to be instantiated or directly used by extensions. This change makes them more similar to and aligned with the `ByteArrayConverter`. It also gives us the opportunity to move them into the “api” module in the future (keeping the same package name), should we ever want or need to do so. However, if we were to start out with them in the “api” module, we would never be able to move them out into the “runtime” module, even if we kept the same package name. Therefore, moving them to “runtime” now gives us a bit more flexibility. This PR moves the unit tests for the numeric converters accordingly, and updates the `PluginsUtil` and `PluginUtilsTest` as well. Author: Randall Hauch <rhauch@gmail.com> Reviewers: Konstantine Karantasis <konstantine@confluent.io>, Ewen Cheslack-Postava <ewen@confluent.io> Closes #5222 from rhauch/kafka-7056 (cherry picked from commit fab8b7e) Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>
KIP-305 added numeric converters to Connect, but these were added in Connect’s API module in the same package as the `StringConverter`. This commit moves them into the Runtime module and into the `converters` package where the `ByteArrayConverter` already lives. These numeric converters have not yet been included in a release, and so they can be moved without concern. All of Connect’s converters must be referenced in worker / connector configurations and are therefore part of the API, but otherwise do not need to be in the “api” module as they do not need to be instantiated or directly used by extensions. This change makes them more similar to and aligned with the `ByteArrayConverter`. It also gives us the opportunity to move them into the “api” module in the future (keeping the same package name), should we ever want or need to do so. However, if we were to start out with them in the “api” module, we would never be able to move them out into the “runtime” module, even if we kept the same package name. Therefore, moving them to “runtime” now gives us a bit more flexibility. This PR moves the unit tests for the numeric converters accordingly, and updates the `PluginsUtil` and `PluginUtilsTest` as well. Author: Randall Hauch <rhauch@gmail.com> Reviewers: Konstantine Karantasis <konstantine@confluent.io>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#5222 from rhauch/kafka-7056
KIP-305 added numeric converters to Connect, but these were added in Connect’s API module in the same package as the
StringConverter
. This commit moves them into the Runtime module and into theconverters
package where theByteArrayConverter
already lives. These numeric converters have not yet been included in a release, and so they can be moved without concern.All of Connect’s converters must be referenced in worker / connector configurations and are therefore part of the API, but otherwise do not need to be in the “api” module as they do not need to be instantiated or directly used by extensions. This change makes them more similar to and aligned with the
ByteArrayConverter
.It also gives us the opportunity to move them into the “api” module in the future (keeping the same package name), should we ever want or need to do so. However, if we were to start out with them in the “api” module, we would never be able to move them out into the “runtime” module, even if we kept the same package name. Therefore, moving them to “runtime” now gives us a bit more flexibility.
This PR moves the unit tests for the numeric converters accordingly, and updates the
PluginsUtil
andPluginUtilsTest
as well.