ableegoldman commented on code in PR #14681:
URL: https://github.com/apache/kafka/pull/14681#discussion_r1380884002
##########
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java:
##########
@@ -618,7 +618,7 @@ public ProducerConfig(Map<String, Object> props) {
super(CONFIG, props);
}
- ProducerConfig(Map<?, ?> props, boolean doLog) {
+ protected ProducerConfig(Map<?, ?> props, boolean doLog) {
Review Comment:
Personally I suppose I can see the argument that `protected` APIs in a
public and non-final class are themselves part of the public API, as one is
technically free to extend that class and would then need to (or at least be
able to) leverage those APIs.
However...I think one could also argue that the public API is only what is
directly accessible to the user, and that if you need to extend a public class
in order to access a given API, then that API is not strictly speaking part of
the API as it is only indirectly accessible from a custom user-defined class.
Imo this is more of a hack/breach of internal APIs than a legitimate use of the
public API -- as a user, I would not expect my custom implementation to have
any guarantees or to be covered by the public contract, and would considered it
my own responsibility/fault if my code failed to compile after an upgrade
because there was a change to the `protected` API I was sneakily using via
inheritance.
Just my own two cents about the situation: that frankly, accessing protected
classes via inheritance is closer in spirit to accessing them (or even private
methods) via reflection, than it is to accessing a true public API covered by
the public contract. Just because one can leverage a feature of Java to gain
access to an API, that doesn't make it a public API in itself.
All that said, I am still happy to do a KIP if that will be the path of
least resistance. @ijuma just let me know because I don't want to merge
something that you would feel the need to revert 🙂
##########
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java:
##########
@@ -618,7 +618,7 @@ public ProducerConfig(Map<String, Object> props) {
super(CONFIG, props);
}
- ProducerConfig(Map<?, ?> props, boolean doLog) {
+ protected ProducerConfig(Map<?, ?> props, boolean doLog) {
Review Comment:
Personally I suppose I can see the argument that `protected` APIs in a
public and non-final class are themselves part of the public API, as one is
technically free to extend that class and would then need to (or at least be
able to) leverage those APIs.
However...I think one could also argue that the public API is only what is
directly accessible to the user, and that if you need to extend a public class
in order to access a given API, then that API is not strictly speaking part of
the API as it is only indirectly accessible from a custom user-defined class.
Imo this is more of a hack/breach of internal APIs than a legitimate use of the
public API -- as a user, I would not expect my custom implementation to have
any guarantees or to be covered by the public contract, and would considered it
my own responsibility/fault if my code failed to compile after an upgrade
because there was a change to the `protected` API I was sneakily using via
inheritance.
Just my own two cents about the situation: that frankly, accessing protected
classes via inheritance is closer in spirit to accessing them (or even private
methods) via reflection, than it is to accessing a true public API covered by
the public contract. Just because one can leverage a feature of Java to gain
access to an API, that doesn't make it a public API in itself.
All that said, I am still happy to do a KIP if that will be the path of
least resistance. @ijuma just let me know because I don't want to merge
something that you would feel the need to revert 🙂
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
