[DISCUSS] Flink annotation strategy/consensus
Hi devs, While I was joining the flink-avro enhancement and cleanup discussion driven by Becket[1], I realized that there are some issues with the current Flink API annotation usage in the source code. As far as I am concerned, Flink wants to control the access/visibility of APIs across modules and for downstreams. Since no OSGI is used(it should not be used because of its complexity, IMHO), Flink decided to use a very lightweight but manual solution: customized annotation like @Internal, @Experimental, @PublicEvolving, etc. This is a Flink only concept on top of JDK annotation and is therefore orthogonal to @Deprecated or any other annotations offered by JDK. After this concept has been used, APIs without one of these annotations are in the kind of gray area which means they have no contract in the context of this new concept. Without any given metadata they could be considered as @Internal or @Experimental, because changes are allowed to be applied at any time. But there is no clear definition and therefore different people will understand it differently. There are two options to improve it, as far as I could figure out: option 1: All APIs must have one of those annotations. We should put some effort into going through all source code and add missing annotations. There were discussions[2] and activities going in this direction. option 2: the community comes to a new consensus that APIs without annotation equals one of @Internal, @Experimental, or @PublicEvolving. I personally will choose @Internal, because it is the safest one. And if @Internal is chosen as the default one, it could also be deprecated, because no annotation equals @Internal. If it makes sense, I can create a FLIP and help the community reach this consensus. Both options have their own pros and cons. I would choose option 2, since we will not end up with a lot of APIs marked as @Internal. Looking forward to hearing your thoughts. Best regards Jing [1] https://lists.apache.org/thread/7zsv528swbjxo5zk0bxq33hrkvd77d6f [2] https://lists.apache.org/thread/zl2rmodsjsdb49tt4hn6wv3gdwo0m31o
Re: [DISCUSS] Flink annotation strategy/consensus
Hi Jing, Thanks for bringing up the discussion. My two cents: 1. All the public methods / classes / interfaces MUST be annotated with one of the @Experimental / @PublicEvolving / @Public. In practice, all the methods by default inherit the annotation from the containing class, unless annotated otherwise. e.g. an @Internal method in a @Public class. 2. I agree it would be too verbose to annotate every internal method / class / interface. Currently we already treat the methods / interfaces / classes without annotations as effectively @Internal. 3. Per our discussion in the other thread, @Deprecated SHOULD coexist with one of the @Experimental / @PublicEvolving / @Public. In that case, @Deprecated overrides the other annotation, which means that public API will not evolve and will be removed according to the deprecation process. 4. The internal methods / classes / interfaces SHOULD NOT be marked as deprecated. Instead, an immediate refactor should be done to remove the "deprecated" internal methods / classes / interfaces, and migrate the code to its successor. Otherwise, technical debts will build up. Thanks, Jiangjie (Becket) Qin On Sat, Sep 9, 2023 at 5:29 AM Jing Ge wrote: > Hi devs, > > While I was joining the flink-avro enhancement and cleanup discussion > driven by Becket[1], I realized that there are some issues with the current > Flink API annotation usage in the source code. > > As far as I am concerned, Flink wants to control the access/visibility of > APIs across modules and for downstreams. Since no OSGI is used(it should > not be used because of its complexity, IMHO), Flink decided to use a very > lightweight but manual solution: customized annotation like @Internal, > @Experimental, @PublicEvolving, > etc. This is a Flink only concept on top of JDK annotation and is therefore > orthogonal to @Deprecated or any other annotations offered by JDK. After > this concept has been used, APIs without one of these annotations are in > the kind of gray area which means they have no contract in the context of > this new concept. Without any given metadata they could be considered > as @Internal or @Experimental, because changes are allowed to be applied at > any time. But there is no clear definition and therefore different people > will understand it differently. > > There are two options to improve it, as far as I could figure out: > > option 1: All APIs must have one of those annotations. We should put some > effort into going through all source code and add missing annotations. > There were discussions[2] and activities going in this direction. > option 2: the community comes to a new consensus that APIs without > annotation equals one of @Internal, @Experimental, or @PublicEvolving. I > personally will choose @Internal, because it is the safest one. And if > @Internal is chosen as the default one, it could also be deprecated, > because no annotation equals @Internal. If it makes sense, I can create a > FLIP and help the community reach this consensus. > > Both options have their own pros and cons. I would choose option 2, since > we will not end up with a lot of APIs marked as @Internal. > > Looking forward to hearing your thoughts. > > Best regards > Jing > > > [1] https://lists.apache.org/thread/7zsv528swbjxo5zk0bxq33hrkvd77d6f > [2] https://lists.apache.org/thread/zl2rmodsjsdb49tt4hn6wv3gdwo0m31o >
Re: [DISCUSS] Flink annotation strategy/consensus
Thanks Jing for starting this discussion. For @Becket > 1. All the public methods / classes / interfaces MUST be annotated with one of the @Experimental / @PublicEvolving / @Public. In practice, all the methods by default inherit the annotation from the containing class, unless annotated otherwise. e.g. an @Internal method in a @Public class. Do we really need to have `@Internal` methods in an `@Public` interface or class? In general, if a class or interface is marked as `@Public `, it is better that their public methods should also be `@Public`, because even if marked as `@Internal`, users are not aware of it when using it, which could be strange. @Jing Besides `@Internal`, I have some cents about `@PublicEvolving` and `@Public`. Currently when we add an interface which will be used by external systems, we often annotate it as `@PublicEvolving`. But when should this interface be marked as `@Public`? I find it is difficult to determine this. Is `@PublicEvolving` really necessary? Should we directly remove `@PublicEvolving` and use `@Public` instead? I think it would be simpler. Best, Shammon FY On Mon, Sep 11, 2023 at 11:05 AM Becket Qin wrote: > Hi Jing, > > Thanks for bringing up the discussion. My two cents: > > 1. All the public methods / classes / interfaces MUST be annotated with one > of the @Experimental / @PublicEvolving / @Public. In practice, all the > methods by default inherit the annotation from the containing class, unless > annotated otherwise. e.g. an @Internal method in a @Public class. > > 2. I agree it would be too verbose to annotate every internal method / > class / interface. Currently we already treat the methods / interfaces / > classes without annotations as effectively @Internal. > > 3. Per our discussion in the other thread, @Deprecated SHOULD coexist with > one of the @Experimental / @PublicEvolving / @Public. In that > case, @Deprecated overrides the other annotation, which means that public > API will not evolve and will be removed according to the deprecation > process. > > 4. The internal methods / classes / interfaces SHOULD NOT be marked as > deprecated. Instead, an immediate refactor should be done to remove the > "deprecated" internal methods / classes / interfaces, and migrate the code > to its successor. Otherwise, technical debts will build up. > > Thanks, > > Jiangjie (Becket) Qin > > > > On Sat, Sep 9, 2023 at 5:29 AM Jing Ge wrote: > > > Hi devs, > > > > While I was joining the flink-avro enhancement and cleanup discussion > > driven by Becket[1], I realized that there are some issues with the > current > > Flink API annotation usage in the source code. > > > > As far as I am concerned, Flink wants to control the access/visibility of > > APIs across modules and for downstreams. Since no OSGI is used(it should > > not be used because of its complexity, IMHO), Flink decided to use a very > > lightweight but manual solution: customized annotation like @Internal, > > @Experimental, @PublicEvolving, > > etc. This is a Flink only concept on top of JDK annotation and is > therefore > > orthogonal to @Deprecated or any other annotations offered by JDK. After > > this concept has been used, APIs without one of these annotations are in > > the kind of gray area which means they have no contract in the context of > > this new concept. Without any given metadata they could be considered > > as @Internal or @Experimental, because changes are allowed to be applied > at > > any time. But there is no clear definition and therefore different people > > will understand it differently. > > > > There are two options to improve it, as far as I could figure out: > > > > option 1: All APIs must have one of those annotations. We should put some > > effort into going through all source code and add missing annotations. > > There were discussions[2] and activities going in this direction. > > option 2: the community comes to a new consensus that APIs without > > annotation equals one of @Internal, @Experimental, or @PublicEvolving. I > > personally will choose @Internal, because it is the safest one. And if > > @Internal is chosen as the default one, it could also be deprecated, > > because no annotation equals @Internal. If it makes sense, I can create a > > FLIP and help the community reach this consensus. > > > > Both options have their own pros and cons. I would choose option 2, since > > we will not end up with a lot of APIs marked as @Internal. > > > > Looking forward to hearing your thoughts. > > > > Best regards > > Jing > > > > > > [1] https://lists.apache.org/thread/7zsv528swbjxo5zk0bxq33hrkvd77d6f > > [2] https://lists.apache.org/thread/zl2rmodsjsdb49tt4hn6wv3gdwo0m31o > > >
Re: [DISCUSS] Flink annotation strategy/consensus
Hi Shammon, Thanks for the reply. Do we really need to have `@Internal` methods in an `@Public` interface or > class? In general, if a class or interface is marked as `@Public `, it is > better that their public methods should also be `@Public`, because even if > marked as `@Internal`, users are not aware of it when using it, which could > be strange. It is more like a legacy issue that the public and internal usage share the same concrete class. e.g. DataStream.getId() is for internal usage, but happens to be in DataStream which is a public class. This should be avoided in the future. It is a good practice to create separate interfaces should be created for the users in this case. Regarding the API stability promotion, you may want to check the FLIP-197[1]. Thanks, Jiangjie (Becket) Qin [1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-197%3A+API+stability+graduation+process On Mon, Sep 11, 2023 at 11:43 AM Shammon FY wrote: > Thanks Jing for starting this discussion. > > For @Becket > > 1. All the public methods / classes / interfaces MUST be annotated with > one of the @Experimental / @PublicEvolving / @Public. In practice, all the > methods by default inherit the annotation from the containing class, unless > annotated otherwise. e.g. an @Internal method in a @Public class. > > Do we really need to have `@Internal` methods in an `@Public` interface or > class? In general, if a class or interface is marked as `@Public `, it is > better that their public methods should also be `@Public`, because even if > marked as `@Internal`, users are not aware of it when using it, which could > be strange. > > @Jing Besides `@Internal`, I have some cents about `@PublicEvolving` and > `@Public`. Currently when we add an interface which will be used by > external systems, we often annotate it as `@PublicEvolving`. But when > should this interface be marked as `@Public`? I find it is difficult to > determine this. Is `@PublicEvolving` really necessary? Should we directly > remove `@PublicEvolving` and use `@Public` instead? I think it would be > simpler. > > Best, > Shammon FY > > > On Mon, Sep 11, 2023 at 11:05 AM Becket Qin wrote: > > > Hi Jing, > > > > Thanks for bringing up the discussion. My two cents: > > > > 1. All the public methods / classes / interfaces MUST be annotated with > one > > of the @Experimental / @PublicEvolving / @Public. In practice, all the > > methods by default inherit the annotation from the containing class, > unless > > annotated otherwise. e.g. an @Internal method in a @Public class. > > > > 2. I agree it would be too verbose to annotate every internal method / > > class / interface. Currently we already treat the methods / interfaces / > > classes without annotations as effectively @Internal. > > > > 3. Per our discussion in the other thread, @Deprecated SHOULD coexist > with > > one of the @Experimental / @PublicEvolving / @Public. In that > > case, @Deprecated overrides the other annotation, which means that public > > API will not evolve and will be removed according to the deprecation > > process. > > > > 4. The internal methods / classes / interfaces SHOULD NOT be marked as > > deprecated. Instead, an immediate refactor should be done to remove the > > "deprecated" internal methods / classes / interfaces, and migrate the > code > > to its successor. Otherwise, technical debts will build up. > > > > Thanks, > > > > Jiangjie (Becket) Qin > > > > > > > > On Sat, Sep 9, 2023 at 5:29 AM Jing Ge > wrote: > > > > > Hi devs, > > > > > > While I was joining the flink-avro enhancement and cleanup discussion > > > driven by Becket[1], I realized that there are some issues with the > > current > > > Flink API annotation usage in the source code. > > > > > > As far as I am concerned, Flink wants to control the access/visibility > of > > > APIs across modules and for downstreams. Since no OSGI is used(it > should > > > not be used because of its complexity, IMHO), Flink decided to use a > very > > > lightweight but manual solution: customized annotation like @Internal, > > > @Experimental, @PublicEvolving, > > > etc. This is a Flink only concept on top of JDK annotation and is > > therefore > > > orthogonal to @Deprecated or any other annotations offered by JDK. > After > > > this concept has been used, APIs without one of these annotations are > in > > > the kind of gray area which means they have no contract in the context > of > > > this new concept. Without any given metadata they could be considered > > > as @Internal or @Experimental, because changes are allowed to be > applied > > at > > > any time. But there is no clear definition and therefore different > people > > > will understand it differently. > > > > > > There are two options to improve it, as far as I could figure out: > > > > > > option 1: All APIs must have one of those annotations. We should put > some > > > effort into going through all source code and add missing annotations. > > > There were discussions[
Re: [DISCUSS] Flink annotation strategy/consensus
Hi Shammon, Thanks for asking. @PublicEvolving is actually a very useful design that developers can offer APIs as publicly accessible but still have changes to introduce modifications afterwards between minor releases. Compared to it, all @Public APIs can only be changed between major releases. Beyond the FLIP-197 mentioned by Becket, you might want to check FLIP-196[1] for further details. Best regards, Jing [1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-196%3A+Source+API+stability+guarantees On Mon, Sep 11, 2023 at 9:46 AM Becket Qin wrote: > Hi Shammon, > > Thanks for the reply. > > Do we really need to have `@Internal` methods in an `@Public` interface or > > class? In general, if a class or interface is marked as `@Public `, it is > > better that their public methods should also be `@Public`, because even > if > > marked as `@Internal`, users are not aware of it when using it, which > could > > be strange. > > It is more like a legacy issue that the public and internal usage share the > same concrete class. e.g. DataStream.getId() is for internal usage, but > happens to be in DataStream which is a public class. This should be avoided > in the future. It is a good practice to create separate interfaces should > be created for the users in this case. > > Regarding the API stability promotion, you may want to check the > FLIP-197[1]. > > Thanks, > > Jiangjie (Becket) Qin > > [1] > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-197%3A+API+stability+graduation+process > > On Mon, Sep 11, 2023 at 11:43 AM Shammon FY wrote: > > > Thanks Jing for starting this discussion. > > > > For @Becket > > > 1. All the public methods / classes / interfaces MUST be annotated with > > one of the @Experimental / @PublicEvolving / @Public. In practice, all > the > > methods by default inherit the annotation from the containing class, > unless > > annotated otherwise. e.g. an @Internal method in a @Public class. > > > > Do we really need to have `@Internal` methods in an `@Public` interface > or > > class? In general, if a class or interface is marked as `@Public `, it is > > better that their public methods should also be `@Public`, because even > if > > marked as `@Internal`, users are not aware of it when using it, which > could > > be strange. > > > > @Jing Besides `@Internal`, I have some cents about `@PublicEvolving` and > > `@Public`. Currently when we add an interface which will be used by > > external systems, we often annotate it as `@PublicEvolving`. But when > > should this interface be marked as `@Public`? I find it is difficult to > > determine this. Is `@PublicEvolving` really necessary? Should we directly > > remove `@PublicEvolving` and use `@Public` instead? I think it would be > > simpler. > > > > Best, > > Shammon FY > > > > > > On Mon, Sep 11, 2023 at 11:05 AM Becket Qin > wrote: > > > > > Hi Jing, > > > > > > Thanks for bringing up the discussion. My two cents: > > > > > > 1. All the public methods / classes / interfaces MUST be annotated with > > one > > > of the @Experimental / @PublicEvolving / @Public. In practice, all the > > > methods by default inherit the annotation from the containing class, > > unless > > > annotated otherwise. e.g. an @Internal method in a @Public class. > > > > > > 2. I agree it would be too verbose to annotate every internal method / > > > class / interface. Currently we already treat the methods / interfaces > / > > > classes without annotations as effectively @Internal. > > > > > > 3. Per our discussion in the other thread, @Deprecated SHOULD coexist > > with > > > one of the @Experimental / @PublicEvolving / @Public. In that > > > case, @Deprecated overrides the other annotation, which means that > public > > > API will not evolve and will be removed according to the deprecation > > > process. > > > > > > 4. The internal methods / classes / interfaces SHOULD NOT be marked as > > > deprecated. Instead, an immediate refactor should be done to remove the > > > "deprecated" internal methods / classes / interfaces, and migrate the > > code > > > to its successor. Otherwise, technical debts will build up. > > > > > > Thanks, > > > > > > Jiangjie (Becket) Qin > > > > > > > > > > > > On Sat, Sep 9, 2023 at 5:29 AM Jing Ge > > wrote: > > > > > > > Hi devs, > > > > > > > > While I was joining the flink-avro enhancement and cleanup discussion > > > > driven by Becket[1], I realized that there are some issues with the > > > current > > > > Flink API annotation usage in the source code. > > > > > > > > As far as I am concerned, Flink wants to control the > access/visibility > > of > > > > APIs across modules and for downstreams. Since no OSGI is used(it > > should > > > > not be used because of its complexity, IMHO), Flink decided to use a > > very > > > > lightweight but manual solution: customized annotation like > @Internal, > > > > @Experimental, @PublicEvolving, > > > > etc. This is a Flink only concept on top of JDK annotation and is > > > there
Re: [DISCUSS] Flink annotation strategy/consensus
Hi Becket, Thanks for your reply with details. > 2. I agree it would be too verbose to annotate every internal method / > class / interface. Currently we already treat the methods / interfaces / > classes without annotations as effectively @Internal. > Does it make sense to clearly define that APIs without annotation are internal APIs and should be used outside of Flink. And deprecate @Internal? Best regards, Jing On Mon, Sep 11, 2023 at 5:05 AM Becket Qin wrote: > Hi Jing, > > Thanks for bringing up the discussion. My two cents: > > 1. All the public methods / classes / interfaces MUST be annotated with one > of the @Experimental / @PublicEvolving / @Public. In practice, all the > methods by default inherit the annotation from the containing class, unless > annotated otherwise. e.g. an @Internal method in a @Public class. > 2. I agree it would be too verbose to annotate every internal method / > class / interface. Currently we already treat the methods / interfaces / > classes without annotations as effectively @Internal. 3. Per our discussion in the other thread, @Deprecated SHOULD coexist with > one of the @Experimental / @PublicEvolving / @Public. In that > case, @Deprecated overrides the other annotation, which means that public > API will not evolve and will be removed according to the deprecation > process. > > 4. The internal methods / classes / interfaces SHOULD NOT be marked as > deprecated. Instead, an immediate refactor should be done to remove the > "deprecated" internal methods / classes / interfaces, and migrate the code > to its successor. Otherwise, technical debts will build up. > > Thanks, > > Jiangjie (Becket) Qin > > > > On Sat, Sep 9, 2023 at 5:29 AM Jing Ge wrote: > > > Hi devs, > > > > While I was joining the flink-avro enhancement and cleanup discussion > > driven by Becket[1], I realized that there are some issues with the > current > > Flink API annotation usage in the source code. > > > > As far as I am concerned, Flink wants to control the access/visibility of > > APIs across modules and for downstreams. Since no OSGI is used(it should > > not be used because of its complexity, IMHO), Flink decided to use a very > > lightweight but manual solution: customized annotation like @Internal, > > @Experimental, @PublicEvolving, > > etc. This is a Flink only concept on top of JDK annotation and is > therefore > > orthogonal to @Deprecated or any other annotations offered by JDK. After > > this concept has been used, APIs without one of these annotations are in > > the kind of gray area which means they have no contract in the context of > > this new concept. Without any given metadata they could be considered > > as @Internal or @Experimental, because changes are allowed to be applied > at > > any time. But there is no clear definition and therefore different people > > will understand it differently. > > > > There are two options to improve it, as far as I could figure out: > > > > option 1: All APIs must have one of those annotations. We should put some > > effort into going through all source code and add missing annotations. > > There were discussions[2] and activities going in this direction. > > option 2: the community comes to a new consensus that APIs without > > annotation equals one of @Internal, @Experimental, or @PublicEvolving. I > > personally will choose @Internal, because it is the safest one. And if > > @Internal is chosen as the default one, it could also be deprecated, > > because no annotation equals @Internal. If it makes sense, I can create a > > FLIP and help the community reach this consensus. > > > > Both options have their own pros and cons. I would choose option 2, since > > we will not end up with a lot of APIs marked as @Internal. > > > > Looking forward to hearing your thoughts. > > > > Best regards > > Jing > > > > > > [1] https://lists.apache.org/thread/7zsv528swbjxo5zk0bxq33hrkvd77d6f > > [2] https://lists.apache.org/thread/zl2rmodsjsdb49tt4hn6wv3gdwo0m31o > > >
Re: [DISCUSS] Flink annotation strategy/consensus
> > Does it make sense to clearly define that APIs without annotation are > internal APIs and should be used outside of Flink. And deprecate @Internal? We can do this. Although I think it is OK to keep the @Internal annotation in case extra clarity is needed sometimes. Thanks, Jiangjie (Becket) Qin On Tue, Sep 12, 2023 at 7:11 PM Jing Ge wrote: > Hi Becket, > > Thanks for your reply with details. > > > > 2. I agree it would be too verbose to annotate every internal method / > > class / interface. Currently we already treat the methods / interfaces / > > classes without annotations as effectively @Internal. > > > > Does it make sense to clearly define that APIs without annotation are > internal APIs and should be used outside of Flink. And deprecate @Internal? > > Best regards, > Jing > > On Mon, Sep 11, 2023 at 5:05 AM Becket Qin wrote: > > > Hi Jing, > > > > Thanks for bringing up the discussion. My two cents: > > > > 1. All the public methods / classes / interfaces MUST be annotated with > one > > of the @Experimental / @PublicEvolving / @Public. In practice, all the > > methods by default inherit the annotation from the containing class, > unless > > annotated otherwise. e.g. an @Internal method in a @Public class. > > > > > 2. I agree it would be too verbose to annotate every internal method / > > class / interface. Currently we already treat the methods / interfaces / > > classes without annotations as effectively @Internal. > > > > 3. Per our discussion in the other thread, @Deprecated SHOULD coexist with > > one of the @Experimental / @PublicEvolving / @Public. In that > > case, @Deprecated overrides the other annotation, which means that public > > API will not evolve and will be removed according to the deprecation > > process. > > > > 4. The internal methods / classes / interfaces SHOULD NOT be marked as > > deprecated. Instead, an immediate refactor should be done to remove the > > "deprecated" internal methods / classes / interfaces, and migrate the > code > > to its successor. Otherwise, technical debts will build up. > > > > Thanks, > > > > Jiangjie (Becket) Qin > > > > > > > > On Sat, Sep 9, 2023 at 5:29 AM Jing Ge > wrote: > > > > > Hi devs, > > > > > > While I was joining the flink-avro enhancement and cleanup discussion > > > driven by Becket[1], I realized that there are some issues with the > > current > > > Flink API annotation usage in the source code. > > > > > > As far as I am concerned, Flink wants to control the access/visibility > of > > > APIs across modules and for downstreams. Since no OSGI is used(it > should > > > not be used because of its complexity, IMHO), Flink decided to use a > very > > > lightweight but manual solution: customized annotation like @Internal, > > > @Experimental, @PublicEvolving, > > > etc. This is a Flink only concept on top of JDK annotation and is > > therefore > > > orthogonal to @Deprecated or any other annotations offered by JDK. > After > > > this concept has been used, APIs without one of these annotations are > in > > > the kind of gray area which means they have no contract in the context > of > > > this new concept. Without any given metadata they could be considered > > > as @Internal or @Experimental, because changes are allowed to be > applied > > at > > > any time. But there is no clear definition and therefore different > people > > > will understand it differently. > > > > > > There are two options to improve it, as far as I could figure out: > > > > > > option 1: All APIs must have one of those annotations. We should put > some > > > effort into going through all source code and add missing annotations. > > > There were discussions[2] and activities going in this direction. > > > option 2: the community comes to a new consensus that APIs without > > > annotation equals one of @Internal, @Experimental, or @PublicEvolving. > I > > > personally will choose @Internal, because it is the safest one. And if > > > @Internal is chosen as the default one, it could also be deprecated, > > > because no annotation equals @Internal. If it makes sense, I can > create a > > > FLIP and help the community reach this consensus. > > > > > > Both options have their own pros and cons. I would choose option 2, > since > > > we will not end up with a lot of APIs marked as @Internal. > > > > > > Looking forward to hearing your thoughts. > > > > > > Best regards > > > Jing > > > > > > > > > [1] https://lists.apache.org/thread/7zsv528swbjxo5zk0bxq33hrkvd77d6f > > > [2] https://lists.apache.org/thread/zl2rmodsjsdb49tt4hn6wv3gdwo0m31o > > > > > >
Re: [DISCUSS] Flink annotation strategy/consensus
We need the @Internal annotation for cases where a user-facing class has methods entirely meant for internal uses. It's not great that we have these cases to being with, but fixing that is another story. The semantics for non-annotated classes is already documented. https://nightlies.apache.org/flink/flink-docs-release-1.17/docs/ops/upgrading/#api-compatibility-guarantees On 14/09/2023 02:31, Becket Qin wrote: Does it make sense to clearly define that APIs without annotation are internal APIs and should be used outside of Flink. And deprecate @Internal? We can do this. Although I think it is OK to keep the @Internal annotation in case extra clarity is needed sometimes. Thanks, Jiangjie (Becket) Qin On Tue, Sep 12, 2023 at 7:11 PM Jing Ge wrote: Hi Becket, Thanks for your reply with details. 2. I agree it would be too verbose to annotate every internal method / class / interface. Currently we already treat the methods / interfaces / classes without annotations as effectively @Internal. Does it make sense to clearly define that APIs without annotation are internal APIs and should be used outside of Flink. And deprecate @Internal? Best regards, Jing On Mon, Sep 11, 2023 at 5:05 AM Becket Qin wrote: Hi Jing, Thanks for bringing up the discussion. My two cents: 1. All the public methods / classes / interfaces MUST be annotated with one of the @Experimental / @PublicEvolving / @Public. In practice, all the methods by default inherit the annotation from the containing class, unless annotated otherwise. e.g. an @Internal method in a @Public class. 2. I agree it would be too verbose to annotate every internal method / class / interface. Currently we already treat the methods / interfaces / classes without annotations as effectively @Internal. 3. Per our discussion in the other thread, @Deprecated SHOULD coexist with one of the @Experimental / @PublicEvolving / @Public. In that case, @Deprecated overrides the other annotation, which means that public API will not evolve and will be removed according to the deprecation process. 4. The internal methods / classes / interfaces SHOULD NOT be marked as deprecated. Instead, an immediate refactor should be done to remove the "deprecated" internal methods / classes / interfaces, and migrate the code to its successor. Otherwise, technical debts will build up. Thanks, Jiangjie (Becket) Qin On Sat, Sep 9, 2023 at 5:29 AM Jing Ge wrote: Hi devs, While I was joining the flink-avro enhancement and cleanup discussion driven by Becket[1], I realized that there are some issues with the current Flink API annotation usage in the source code. As far as I am concerned, Flink wants to control the access/visibility of APIs across modules and for downstreams. Since no OSGI is used(it should not be used because of its complexity, IMHO), Flink decided to use a very lightweight but manual solution: customized annotation like @Internal, @Experimental, @PublicEvolving, etc. This is a Flink only concept on top of JDK annotation and is therefore orthogonal to @Deprecated or any other annotations offered by JDK. After this concept has been used, APIs without one of these annotations are in the kind of gray area which means they have no contract in the context of this new concept. Without any given metadata they could be considered as @Internal or @Experimental, because changes are allowed to be applied at any time. But there is no clear definition and therefore different people will understand it differently. There are two options to improve it, as far as I could figure out: option 1: All APIs must have one of those annotations. We should put some effort into going through all source code and add missing annotations. There were discussions[2] and activities going in this direction. option 2: the community comes to a new consensus that APIs without annotation equals one of @Internal, @Experimental, or @PublicEvolving. I personally will choose @Internal, because it is the safest one. And if @Internal is chosen as the default one, it could also be deprecated, because no annotation equals @Internal. If it makes sense, I can create a FLIP and help the community reach this consensus. Both options have their own pros and cons. I would choose option 2, since we will not end up with a lot of APIs marked as @Internal. Looking forward to hearing your thoughts. Best regards Jing [1] https://lists.apache.org/thread/7zsv528swbjxo5zk0bxq33hrkvd77d6f [2] https://lists.apache.org/thread/zl2rmodsjsdb49tt4hn6wv3gdwo0m31o
