ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nikolay Izhikov <nizhi...@apache.org>
Subject Re: Internal classes are exposed in public API
Date Fri, 17 Jan 2020 20:28:15 GMT
+1 to mark feature or whole release as EA.

пт, 17 янв. 2020 г., 23:00 Denis Magda <dmagda@apache.org>:

> Folks, if you don't mind I'll share some thoughts/suggestions as an
> observer who was not involved in the feature development.
>
> It's absolutely 'ok' to deprecate an API that is replaced with a much
> better version. However, we should do this only when the new APIs are
> production-ready. If there are still many limitations or open items then
> don't deprecate anything that exists and release the new APIs labeling as
> early access. What if release the improvements labeling as EA instead of
> hiding them completely?
>
> I would also encourage us to put aside emotions, don't blame or point
> fingers. This IEP is a great initiative and you both have already done a
> tremendous job by developing, architecting and reviewing changes. Just be
> respectful. Nobody is trying to block the feature from being released.
> Everyone would be glad to tap into improvements and start using them in
> prod. But if more time is needed for the GA let's reiterate a bit.
>
> -
> Denis
>
>
> On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков <nizhikov@apache.org>
> wrote:
>
> > > Also I agree with Alexey about introducing public IgniteMonitoring
> facade
> >
> > This is not an issue with the API.
> > Just the new feature that doesn’t affects API.
> > Moreover, I create a ticket to fix it, already.
> >
> > > It's right. But if you add checking of statisticsEnabling property then
> > test will fail. It's just not good tests.
> >
> > My changes doesn’t affect any `staticticsEnabled` property.
> > I don’ understand your point here.
> >
> > > Redundant ReadOnlyMetricRegistry.
> >
> > It’s not redundant.
> > It required for exporters and provide read only access to MetricRegistry
> > existing in the node.
> >
> >
> > > MetricExporterSpi that requires ReadOnlyMetricRegistry.
> >
> > > Absence of newly created metrics in old MBeans that forces user use
> > > exporter SPI while his code base uses old MBeans.
> >
> > Why this is issue?
> >
> > > Inconsistent MetricRegistry API.
> >
> > It’s consistent.
> >
> > > Metrics lookups from map instead of using direct reference
> > > (performance problem).
> >
> > 1. We(You and I) did this choice together to simplify creation of the new
> > metrics.
> > 2. This is not public API issue.
> >
> >
> > > Ignoring of statisticsEnabled flag.
> >
> > I don’t ignore this flag.
> > It just doesn’t exists in new framework(because of scope).
> > I don’t think it’s an issue.
> >
> > > JmxExporterSpi creates beans that doesn't satisfy best MBeans
> practices.
> >
> > Please, clarify your statement.
> > What is best MBeans practices you are talking about?
> >
> > > Not finished IGNITE-11927
> >
> >
> > How this can be API issue?
> >
> >
> > > 17 янв. 2020 г., в 20:52, Andrey Gura <agura@apache.org> написал(а):
> > >
> > >>> All issues Alexey mentioned in starting letter are fixed with my PR
> > [1].
> > >>> I don’t think other issues you mentioned are blocker for the release.
> > >
> > > As I mentioned already IGNITE-11927 is part of IEP-35 and
> > > implementation seriously affects API's. Also I agree with Alexey about
> > > introducing public IgniteMonitoring facade. So thiss PR doesn't fix
> > > all issues.
> > >
> > >>> I talk about ignored existing functionality.
> > >> There is no existing tests that was broken by this contribution.
> > >
> > > It's right. But if you add checking of statisticsEnabling property
> > > then test will fail. It's just not good tests.
> > >
> > >> If you know the issues with it, feel free to create a ticket I will
> fix
> > it ASAP.
> > >
> > > I already fix it all in IGNITE-11927
> > >
> > >>> 1. Moving IEP-35 API's to the internal package.
> > >> We should move the product forward and shouldn't hide major
> > contribution from the user just because your second guess «I don’t like
> the
> > API I just reviewed and approved».
> > >
> > > We should move the product forward with with really finished features,
> > > not pieces of features.
> > >
> > >> So I am against this proposal.
> > >
> > > It is not argument.
> > >
> > >> But, I’m ready to see your proposal for the API change and discuss
> them.
> > >
> > > We can prepare it together. But we can't block release.
> > >
> > >>> Because IGNITE-11927 doesn't solve all problems
> > >> What is *ALL* problems?
> > >
> > > Redundant ReadOnlyMetricRegistry.
> > > MetricExporterSpi that requires ReadOnlyMetricRegistry.
> > > Absence of newly created metrics in old MBeans that forces user use
> > > exporter SPI while his code base uses old MBeans.
> > > Inconsistent MetricRegistry API.
> > > Metrics lookups from map instead of using direct reference
> > > (performance problem).
> > > Ignoring of statisticsEnabled flag.
> > > JmxExporterSpi creates beans that doesn't satisfy best MBeans
> practices.
> > > Not finished IGNITE-11927
> > >
> > > It's enough I believe.
> > >
> > > On Fri, Jan 17, 2020 at 7:28 PM Николай Ижиков <nizhikov@apache.org>
> > wrote:
> > >>
> > >> Andrey.
> > >>
> > >> All issues Alexey mentioned in starting letter are fixed with my PR
> [1].
> > >> I don’t think other issues you mentioned are blocker for the release.
> > >>
> > >>> I talk about ignored existing functionality.
> > >>
> > >> There is no existing tests that was broken by this contribution.
> > >> If you know the issues with it, feel free to create a ticket I will
> fix
> > it ASAP.
> > >>
> > >>> 1. Moving IEP-35 API's to the internal package.
> > >>
> > >> We should move the product forward and shouldn't hide major
> > contribution from the user just because your second guess «I don’t like
> the
> > API I just reviewed and approved».
> > >> So I am against this proposal.
> > >> But, I’m ready to see your proposal for the API change and discuss
> them.
> > >>
> > >>> Because IGNITE-11927 doesn't solve all problems
> > >>
> > >> What is *ALL* problems?
> > >> Seems, we never be able to solve *ALL* problems.
> > >> But, we should move the product forward.
> > >>
> > >> As for your steps [1-6].
> > >> I’m always following these steps during my contribution.
> > >>
> > >> [1] https://github.com/apache/ignite/pull/7269
> > >>
> > >>> 17 янв. 2020 г., в 19:08, Andrey Gura <agura@apache.org>
написал(а):
> > >>>
> > >>> The discussion is hot and can be endless. So in separate post I want
> > >>> propose my solution.
> > >>>
> > >>> 1. Moving IEP-35 API's to the internal package.
> > >>> 2. Create special feature branch B.
> > >>> 3. In branch B will be merged IGNITE-11927
> > >>> 4. Because IGNITE-11927 doesn't solve all problems we should propose
> > >>> solution and implement it in branch B.
> > >>> 5. Testing, usability testing, fixing, etc iteratively.
> > >>> 6. Merge it to master and in new release branch if needed.
> > >>>
> > >>> Independent step. There are some problem which should be fixed in 2.8
> > >>> before release otherwise we introduce problems with compatibility
> > >>> which will haunt us till next major release. I'll create tickets.
> > >>>
> > >>> On Fri, Jan 17, 2020 at 7:03 PM Andrey Gura <agura@apache.org>
> wrote:
> > >>>>
> > >>>>>> Because it is brand new API and it requires rewrite client
code.
> > >>>>> We doesn’t break backward compatibility.
> > >>>>> The message is - this interface would be remove in the next
major
> > release.
> > >>>>
> > >>>> We don't know anything about development processes our users. I
can
> > >>>> admit that process could require that deprecated methods/APIs are
> not
> > >>>> allowed for example.
> > >>>>
> > >>>>>> ReadOnlyMetricRegistry
> > >>>>>> Form user stand point it is very strange interface which
don't
> give
> > me any information about it’s purpose and responsibilities.
> > >>>>>> Seems, I should explain proposed changes [1] more clear:
> > >>>>
> > >>>> I understand this. But I'm not Ignite user, I'm Ignite developer.
> The
> > >>>> key moment in my message *from user stand point*. From my point
of
> > >>>> view it is very not intuitive solution and this interface is
> > >>>> redundant.
> > >>>>
> > >>>>>> Actually not. We have statisticsEnabled for caches for
example.
> > There are other entities with such flag.
> > >>>>> They still works as expected.
> > >>>>
> > >>>> Actually not. I fixed many such issues during IGNITE-11927
> > implementation.
> > >>>>
> > >>>>>> Why do you decided do in such way?
> > >>>>> Because of the scope.
> > >>>>> The ability to disable/enable metrics is the matter of the
other
> > ticket.
> > >>>>
> > >>>> I talk not about ability. I talk about ignored existing
> functionality.
> > >>>> So scope is not case here.
> > >>>>
> > >>>>>> But they should not be exported by MetricExporterSpi
> > implementations.
> > >>>>> Actually, it’s a responsibility of the exporter to decide.
> > >>>>> JMX exporter can exports ObjectMetric while OpenCensus exporter
> > can’t.
> > >>>>
> > >>>> Actually list is not metric at all as I already told.
> > >>>>
> > >>>> On Fri, Jan 17, 2020 at 5:26 PM Николай Ижиков <nizhikov@apache.org
> >
> > wrote:
> > >>>>>
> > >>>>> Andrey.
> > >>>>>
> > >>>>>> Because it is brand new API and it requires rewrite client
code.
> > >>>>>
> > >>>>> We doesn’t break backward compatibility.
> > >>>>> The message is - this interface would be remove in the next
major
> > release.
> > >>>>>
> > >>>>>> ReadOnlyMetricRegistry
> > >>>>>> Form user stand point it is very strange interface which
don't
> give
> > me any information about it’s purpose and responsibilities.
> > >>>>>
> > >>>>> Seems, I should explain proposed changes [1] more clear:
> > >>>>>
> > >>>>> Each SPI would have a `ReadOnlyMetricManager` which provides
access
> > to collection of `ReadOnlyMetricRegistry`
> > >>>>> which has a collection of `Metric`.
> > >>>>>
> > >>>>> So we reflects two-level structure we have in the internal
API
> > >>>>>
> > >>>>> GridMetricManager -> Collection[MetricRegistry] ->
> Collection[Metric]
> > >>>>> ReadOnlyMetricManager -> Collection[ReadOnlyMetricRegistry]
->
> > Collection[Metric]
> > >>>>>
> > >>>>>> Actually not. We have statisticsEnabled for caches for
example.
> > There are other entities with such flag.
> > >>>>>
> > >>>>> They still works as expected.
> > >>>>>
> > >>>>>> Why do you decided do in such way?
> > >>>>>
> > >>>>> Because of the scope.
> > >>>>> The ability to disable/enable metrics is the matter of the
other
> > ticket.
> > >>>>>
> > >>>>>> But they should not be exported by MetricExporterSpi
> > implementations.
> > >>>>>
> > >>>>> Actually, it’s a responsibility of the exporter to decide.
> > >>>>> JMX exporter can exports ObjectMetric while OpenCensus exporter
> > can’t.
> > >>>>>
> > >>>>> [1]
> >
> https://github.com/apache/ignite/pull/7269/files#diff-0ae5657231fc4c1f650493b02190b81bR25
> > >>>>>
> > >>>>>> 17 янв. 2020 г., в 16:57, Andrey Gura <agura@apache.org>
> > написал(а):
> > >>>>>>
> > >>>>>>> If I’m not missing something, you were one of the
active
> reviewers
> > of the Metric API.
> > >>>>>>
> > >>>>>> Yes. But if I'm not missing some thing you were major developer
of
> > >>>>>> Metric API :) Shit happens. And it happened.
> > >>>>>>
> > >>>>>>>> The first, I agree with Alexey about deprecation
of APIs that
> are
> > still supported and don't offer reasonable substitution.
> > >>>>>>> It has - MetricExporterSPI.
> > >>>>>>
> > >>>>>> There is such concept - backward compatibility. I understand
that
> > >>>>>> deprecation of some interface doesn't break backward compatibility
> > but
> > >>>>>> it leads to question^ what should I use instead of this.
And
> > >>>>>> MetricExporterSpi is not answer for this question. Because
it is
> > brand
> > >>>>>> new API and it requires rewrite client code.
> > >>>>>>
> > >>>>>>>> ReadOnlyMetricRegistry interface is redundant.
> > >>>>>>> It’s an interface that exposes internal MetricRegistry
 to the
> > exporters.
> > >>>>>>
> > >>>>>> No it is not. It's completely artificial thing which allow
iterate
> > via
> > >>>>>> all metric registries. GridMetricManager implements this
interface
> > >>>>>> while it is not metric registry. Form user stand point
it is very
> > >>>>>> strange interface which don't give me any information about
it's
> > >>>>>> purpose and responsibilities.
> > >>>>>>
> > >>>>>>>> Exporters expose metrics if they are disabled.
> > >>>>>>> We don’t have an ability to disable metrics.
> > >>>>>>
> > >>>>>> Actually not. We have statisticsEnabled for caches for
example.
> > There
> > >>>>>> are other entities with such flag.
> > >>>>>>
> > >>>>>>> And that done, intentionally.
> > >>>>>>
> > >>>>>> Why do you decided do in such way? Why you ignore existing
> > >>>>>> functionality? It affects user expectations and experience.
> > >>>>>>
> > >>>>>>> You are working on this issue, aren’t you?
> > >>>>>>
> > >>>>>> Yes? I'm working. Unfortunately we are not synchronized
in this
> > >>>>>> context and I should redo all metrics related changes in
order to
> > >>>>>> merge it with my changes. Anyway, my change doesn't solve
all
> > problems
> > >>>>>> (e.g. it doesn't introduce IgniteMonitoring facade).
> > >>>>>>
> > >>>>>>> I can fix this issue, by myself.
> > >>>>>>
> > >>>>>> Unfortunately it will be just fix. In my solution it is
redesign.
> > Stop
> > >>>>>> fixing issues, let's do things. It requires deeper changes.
My
> > changes
> > >>>>>> blocks AI 2.8 release because it big enough. So it retargeted
on
> the
> > >>>>>> next release. And it is one more reason for moving the
changes to
> > the
> > >>>>>> internal packages. And it isn't good news for me because
I will go
> > >>>>>> throughout pan and tiers of merge. But it is right.
> > >>>>>>
> > >>>>>>> Metrics of type lists are not metric at all.
> > >>>>>>
> > >>>>>> They are created to deal with backward compatibility.
> > >>>>>>
> > >>>>>>>> Metrics of type lists are not metric at all.
> > >>>>>>> They are created to deal with backward compatibility.
> > >>>>>>
> > >>>>>> Yes, I know. But they should not be exported by MetricExporterSpi
> > >>>>>> implementations.
> > >>>>>>
> > >>>>>> On Fri, Jan 17, 2020 at 3:37 PM Николай Ижиков
<
> nizhikov@apache.org>
> > wrote:
> > >>>>>>>
> > >>>>>>> Andrey, thanks for your opinion and your ownest critisism.
> > >>>>>>> I can’t wait for your contribution.
> > >>>>>>>
> > >>>>>>> If I’m not missing something, you were one of the
active
> reviewers
> > of the Metric API.
> > >>>>>>>
> > >>>>>>>> The first, I agree with Alexey about deprecation
of APIs that
> are
> > still supported and don't offer reasonable substitution.
> > >>>>>>>
> > >>>>>>> It has - MetricExporterSPI.
> > >>>>>>>
> > >>>>>>>> The second, from my point of view, we can't recommend
> > MetricExporterSpi's because it are still not-production ready.
> > >>>>>>>
> > >>>>>>> It’s ready.
> > >>>>>>>
> > >>>>>>>> The third, moving of MetricRegistry to the public
API doesn't
> > solve the problem because this interface exposes internal Metric
> interface
> > implementations.
> > >>>>>>>
> > >>>>>>> Not, its’ not.
> > >>>>>>> Please, see `org.apache.ignite.spi.metric.LongMetric`
and other
> > public interface.
> > >>>>>>>
> > >>>>>>>> API of MetricRegistry is inconsistent.
> > >>>>>>>
> > >>>>>>> MetricRegistry is the internal API.
> > >>>>>>> Feel free to create ticket for an issues with it and
I will try
> to
> > fix it.
> > >>>>>>>
> > >>>>>>>> ReadOnlyMetricRegistry interface is redundant.
> > >>>>>>>
> > >>>>>>> It’s an interface that exposes internal MetricRegistry
 to the
> > exporters.
> > >>>>>>>
> > >>>>>>>> Exporters expose metrics if they are disabled.
> > >>>>>>>
> > >>>>>>> We don’t have an ability to disable metrics.
> > >>>>>>> And that done, intentionally.
> > >>>>>>> You are working on this issue, aren’t you?
> > >>>>>>> I can fix this issue, by myself.
> > >>>>>>>
> > >>>>>>>> Metrics of type lists are not metric at all.
> > >>>>>>>
> > >>>>>>> They are created to deal with backward compatibility.
> > >>>>>>>
> > >>>>>>>> 17 янв. 2020 г., в 15:09, Andrey Gura <agura@apache.org>
> > написал(а):
> > >>>>>>>>
> > >>>>>>>> Hi,
> > >>>>>>>>
> > >>>>>>>> The first, I agree with Alexey about deprecation
of APIs that
> are
> > >>>>>>>> still supported and don't offer reasonable substitution.
> > >>>>>>>>
> > >>>>>>>> The second, from my point of view, we can't recommend
> > >>>>>>>> MetricExporterSpi's because it are still not-production
ready.
> > There
> > >>>>>>>> are some issues with it and usage of ReadOnlyMetricRegistry
> > interface
> > >>>>>>>> just one of them.
> > >>>>>>>>
> > >>>>>>>> The third, moving of MetricRegistry to the public
API doesn't
> > solve
> > >>>>>>>> the problem because this interface exposes internal
Metric
> > interface
> > >>>>>>>> implementations. So your PR is incomplete.
> > >>>>>>>> Moreover, API of MetricRegistry is inconsistent.
E.g.
> > register(name,
> > >>>>>>>> supplier, desc) method returns registered metric
for some types
> > and
> > >>>>>>>> doesn't for other. register(metric) method is inconsistent
in
> > sense of
> > >>>>>>>> metric naming. ReadOnlyMetricRegistry interface
is redundant.
> > >>>>>>>> MetricExporterSpi should be revised because it
absolutely not
> > >>>>>>>> intuitive because it requires ReadOnlyMetricRegistry
and it's
> > purpose
> > >>>>>>>> is undefined.
> > >>>>>>>>
> > >>>>>>>> One more point. IEP-35 is still not fully implemented.
Some
> > things are
> > >>>>>>>> not taken into account. Exporters expose metrics
if they are
> > disabled.
> > >>>>>>>> JMX beans exposes values that don't confirm to
best practices
> [1].
> > >>>>>>>> Metrics of type lists are not metric at all. Ubiquitous
merics
> > lookup
> > >>>>>>>> from hash map instead of usage reference for getting
metrics
> > values
> > >>>>>>>> (it is just performance issue). Also IGNITE-11927
is not
> > implemented
> > >>>>>>>> which also changes interfaces significantly.
> > >>>>>>>>
> > >>>>>>>> Let's just admit that the implementation is immature
and must be
> > moved
> > >>>>>>>> to the internal packages.
> > >>>>>>>>
> > >>>>>>>> And because we already merged partially implemented
IEP to the
> > master
> > >>>>>>>> branch we *must move all currently public APIs
to the internal
> > API*
> > >>>>>>>> while it will not be ready for publication.
> > >>>>>>>>
> > >>>>>>>> And the last but not least. What is happening indicates
a
> immature
> > >>>>>>>> development process which must be revised. I don't
want discuss
> > it in
> > >>>>>>>> this thread but we must not allow merge of change
to the master
> > branch
> > >>>>>>>> before it will completed, that is we must use feature
branches
> for
> > >>>>>>>> full IEP not only for particular tickets. And also
we should
> > >>>>>>>> reformulate IEP process in order to avoid things
like this.
> > >>>>>>>>
> > >>>>>>>> [1]
> >
> https://www.oracle.com/technetwork/java/javase/tech/best-practices-jsp-136021.html
> > >>>>>>>>
> > >>>>>>>> On Fri, Jan 17, 2020 at 12:49 PM Николай
Ижиков <
> > nizhikov@apache.org> wrote:
> > >>>>>>>>>
> > >>>>>>>>> Alex.
> > >>>>>>>>>
> > >>>>>>>>> OK, I may leverage your experience and create
pure Java API.
> > >>>>>>>>> Ticket [1] created.
> > >>>>>>>>>
> > >>>>>>>>> But, personally, I don’t agree with you.
> > >>>>>>>>> Ignite has dozens of the API that theoretically
have a usage
> > scenario, but in real-world have 0 custom implementation and usages.
> > >>>>>>>>> Moreover, many APIs that were created with
the intentions you
> > mentioned is abandoned now and confuses users.
> > >>>>>>>>>
> > >>>>>>>>> You can just see count of the tests we just
mute on the TC.
> > >>>>>>>>>
> > >>>>>>>>> Can you, please, take a look at the fix regarding
puck API
> issue
> > you mentioned in your first letter [2], [3]
> > >>>>>>>>>
> > >>>>>>>>> [1] https://issues.apache.org/jira/browse/IGNITE-12553
> > >>>>>>>>> [2] https://github.com/apache/ignite/pull/7269
> > >>>>>>>>> [3] https://issues.apache.org/jira/browse/IGNITE-12552
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>> 17 янв. 2020 г., в 12:12, Alexey Goncharuk
<
> > alexey.goncharuk@gmail.com> написал(а):
> > >>>>>>>>>>
> > >>>>>>>>>> Nikolay,
> > >>>>>>>>>>
> > >>>>>>>>>> Why do you think this is a wrong usage
pattern? From the top
> of
> > my head,
> > >>>>>>>>>> here is a few cases of direct metric API
usage that I know are
> > currently
> > >>>>>>>>>> being used in production:
> > >>>>>>>>>> * A custom task execution scheduling service
with load
> > balancing based on
> > >>>>>>>>>> utilization metrics readings from Java
code
> > >>>>>>>>>> * Cleanup task trigger based on metrics
readings
> > >>>>>>>>>> * A custom health-check endpoint for an
application with an
> > embedded
> > >>>>>>>>>> Ignite node for Kubernetes/Spring Application/etc
> > >>>>>>>>>
> > >>>>>>>
> > >>>>>
> > >>
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message