From dev-return-49148-archive-asf-public=cust-asf.ponee.io@ignite.apache.org Fri Jan 17 13:57:52 2020 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 7B4FB180661 for ; Fri, 17 Jan 2020 14:57:49 +0100 (CET) Received: (qmail 47004 invoked by uid 500); 17 Jan 2020 13:57:47 -0000 Mailing-List: contact dev-help@ignite.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@ignite.apache.org Delivered-To: mailing list dev@ignite.apache.org Received: (qmail 46991 invoked by uid 99); 17 Jan 2020 13:57:47 -0000 Received: from Unknown (HELO mailrelay1-lw-us.apache.org) (10.10.3.159) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 17 Jan 2020 13:57:47 +0000 Received: from mail-wm1-f42.google.com (mail-wm1-f42.google.com [209.85.128.42]) by mailrelay1-lw-us.apache.org (ASF Mail Server at mailrelay1-lw-us.apache.org) with ESMTPSA id DBDE41077 for ; Fri, 17 Jan 2020 13:57:45 +0000 (UTC) Received: by mail-wm1-f42.google.com with SMTP id p17so7746094wma.1 for ; Fri, 17 Jan 2020 05:57:45 -0800 (PST) X-Gm-Message-State: APjAAAU+Ny7u8ezukcQDGB/o2y6Cc9b0veA6A4/OnpbKNW8gGZMmwVj+ YMr2+CSJ+6ZEND080Mdf/gScF863UPJz9fFmnUo= X-Google-Smtp-Source: APXvYqzaxIXXtU0q+Sl8LULwfqIwLnfBCC3AaHYsaL/5BqsyD1xzEVXCUYAiVq+w4uzvDNnBO0c9m6u7KjiC2L9GJB8= X-Received: by 2002:a1c:6a07:: with SMTP id f7mr4657333wmc.171.1579269464670; Fri, 17 Jan 2020 05:57:44 -0800 (PST) MIME-Version: 1.0 References: <1F8796A8-09DC-40C5-B29D-76D38D24A825@gmail.com> <6B6F7062-62CB-4E71-AE2E-4A5C2F44962C@gmail.com> <045C845B-2C46-4CE5-8E7B-430001E1F5C2@gmail.com> <879EF751-0C72-4278-B786-49B385133D0E@gmail.com> <101540C7-89CD-4DA7-AC80-87CE31FBBE6E@gmail.com> <25459122-17CE-451C-95AB-3E4CCA597470@gmail.com> In-Reply-To: <25459122-17CE-451C-95AB-3E4CCA597470@gmail.com> From: Andrey Gura Date: Fri, 17 Jan 2020 16:57:33 +0300 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: Internal classes are exposed in public API To: dev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > If I=E2=80=99m not missing something, you were one of the active reviewer= s 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=E2=80=99s an interface that exposes internal MetricRegistry to the ex= porters. 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=E2=80=99t 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=E2=80=99t 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 =D0=9D=D0=B8=D0=BA=D0=BE=D0=BB=D0=B0=D0=B9 = =D0=98=D0=B6=D0=B8=D0=BA=D0=BE=D0=B2 wrote: > > Andrey, thanks for your opinion and your ownest critisism. > I can=E2=80=99t wait for your contribution. > > If I=E2=80=99m not missing something, you were one of the active reviewer= s 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=E2=80=99s ready. > > > The third, moving of MetricRegistry to the public API doesn't solve the= problem because this interface exposes internal Metric interface implement= ations. > > Not, its=E2=80=99 not. > Please, see `org.apache.ignite.spi.metric.LongMetric` and other public in= terface. > > > 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=E2=80=99s an interface that exposes internal MetricRegistry to the ex= porters. > > > Exporters expose metrics if they are disabled. > > We don=E2=80=99t have an ability to disable metrics. > And that done, intentionally. > You are working on this issue, aren=E2=80=99t 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 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 15:09, Andrey Gura =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): > > > > 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 =D0=9D=D0=B8=D0=BA=D0=BE=D0=BB=D0=B0= =D0=B9 =D0=98=D0=B6=D0=B8=D0=BA=D0=BE=D0=B2 wrote: > >> > >> Alex. > >> > >> OK, I may leverage your experience and create pure Java API. > >> Ticket [1] created. > >> > >> But, personally, I don=E2=80=99t 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 mentione= d 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 m= entioned 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 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 12:12, Alexey Goncharuk <= alexey.goncharuk@gmail.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0= =B0): > >>> > >>> Nikolay, > >>> > >>> Why do you think this is a wrong usage pattern? From the top of my he= ad, > >>> here is a few cases of direct metric API usage that I know are curren= tly > >>> being used in production: > >>> * A custom task execution scheduling service with load balancing base= d 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 > >> >