From dev-return-103156-archive-asf-public=cust-asf.ponee.io@kafka.apache.org Fri Apr 12 01:00:34 2019 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 271BB18065D for ; Fri, 12 Apr 2019 03:00:34 +0200 (CEST) Received: (qmail 21460 invoked by uid 500); 12 Apr 2019 01:00:32 -0000 Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@kafka.apache.org Delivered-To: mailing list dev@kafka.apache.org Received: (qmail 21435 invoked by uid 99); 12 Apr 2019 01:00:32 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd4-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 12 Apr 2019 01:00:32 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd4-us-west.apache.org (ASF Mail Server at spamd4-us-west.apache.org) with ESMTP id CFBDEC030E for ; Fri, 12 Apr 2019 01:00:31 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd4-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.542 X-Spam-Level: * X-Spam-Status: No, score=1.542 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.258, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Authentication-Results: spamd4-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd4-us-west.apache.org [10.40.0.11]) (amavisd-new, port 10024) with ESMTP id 3ZE0l5CVmR01 for ; Fri, 12 Apr 2019 01:00:29 +0000 (UTC) Received: from mail-oi1-f196.google.com (mail-oi1-f196.google.com [209.85.167.196]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with ESMTPS id 98E1E5F67F for ; Fri, 12 Apr 2019 01:00:28 +0000 (UTC) Received: by mail-oi1-f196.google.com with SMTP id i21so6547066oib.11 for ; Thu, 11 Apr 2019 18:00:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=OardZqvZQSx9isfa3afb69fUF+ASrJ1z1+qn81xgXMU=; b=d8+mMO1mZlVC99daAUEV8CUotkX1WdDaO9nZjqlc2jrmStnJYNgdSj3TKo46KRVnOv MgVkDQ0H+rTY4FCxpx6BlHRgIIaWRim1w+3/qUhNjCaXgp92CW0hfwsXXmIhqZhOyBvV tjugSvXuKLAUgicbmC/3HQ7F9lrruFhNGrlGgV7RivkBAfvlON/UfpJJJvpXqigmi1Db f1T8qwFwnXSd3rOeSjK8lB3Xy2gt3SMfxMGeEVoZfWJ0MJfyJjLhwPw2Ybf4U6sSkBRu 33bmIf7E8dfdBNYMxwupqnl3/QJ9AMUcM5Xttujy8i3dAF3wNRmk6cdOxfsUBw1Nczib umMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=OardZqvZQSx9isfa3afb69fUF+ASrJ1z1+qn81xgXMU=; b=ns4QvDkESc4+l6zbN7z+6vGWtuB/TQDw8XV6Yl4WJhNSWwbXSUdCE3ftHVoldhV9hK pNt5kXPTcZuBDzq2BquCe+bRk3/kQFEkYN9KI9jYj8PFwfVF48LcqrmaS1Ci50hM8woI DnirK5Aagp78RSP3XXYB+yLuWiCCiJVTCwTyul4Mooxt/uxGOA2geKBKBVCxNG3pMjIQ dR6UTxW61Q2A2r+VzO5AphQ5luU1Qmkr4t7mAjFDeE4vzR2ZlQckFSWSTWYBylJ40qkF ebDnMxiSeMPvoAFx6a/m0Mc1WVVajJ7XguHfusybquFk63u6PFgk8i1FzPjvbJcOmhDh krPA== X-Gm-Message-State: APjAAAWS8x5HMiBFeeOsqXU6e7oerEWP+9bRWhwgc0L9PkYKO/inBl7b 4vsgBXENKiou9JZnmTXiW8Y6S3vTFHOyYrYGMwo5DZsH X-Google-Smtp-Source: APXvYqz3V2m7x6awSyEjzv7xD9+L15VM+CgcmMOJA+PMuE4QkGrrjhK/p6dHrIqCGhexMtzJOY0OAyHU7iLQCdOwmAo= X-Received: by 2002:aca:4308:: with SMTP id q8mr7730261oia.123.1555030826836; Thu, 11 Apr 2019 18:00:26 -0700 (PDT) MIME-Version: 1.0 References: <72ab9534-a25b-b15c-46ae-0849247e9b54@confluent.io> In-Reply-To: From: Guozhang Wang Date: Thu, 11 Apr 2019 18:00:15 -0700 Message-ID: Subject: Re: [DISCUSS] KIP-439: Deprecate Interface WindowStoreIterator To: dev Content-Type: multipart/alternative; boundary="0000000000000c676305864ad4ea" --0000000000000c676305864ad4ea Content-Type: text/plain; charset="UTF-8" While working at KIP-444 (https://github.com/apache/kafka/pull/6498) I realized there are a bunch of issues on metric names v.s. function names, e.g. some function named `fetchAll` are actually measure with `fetch`, etc. So in that KIP I proposed to make the function name aligned with metrics name. So suppose we rename the functions from `fetch` to `range` I'd suggest we make this change as part of KIP-444 as well. Note that it means different functions with the same name `range` will be measured under a single metric then. But still for function named `all` it will be measured under a separate metric named `all`, so I'm just clarifying with you if that's the intention. Guozhang On Thu, Apr 11, 2019 at 2:04 PM Matthias J. Sax wrote: > I did not see a reason to rename `all()` to `range()`. `all()` does not > take any parameters to limit a range and is a good name IMHO. But I am > not married to keep `all()` and if we think we should rename it, too, I > am fine with it. > > Not sure what connection you make to metrics though. Can you elaborate? > > > Would be interested to hear others opinions on this, too. > > > -Matthias > > On 4/11/19 8:38 AM, Guozhang Wang wrote: > > I like the renaming, since it also aligns with our metrics cleanup > > (KIP-444) which touches upon the store level metrics as well. > > > > One question: you seems still suggesting to keep "all" with the current > > name (and also using a separate metric for it), what's the difference > > between this one and other "range" functions? > > > > > > Guozhang > > > > On Thu, Apr 11, 2019 at 2:26 AM Matthias J. Sax > > wrote: > > > >> Thanks for the input. > >> > >>>> Just to clarify the naming conflicts is between the newly added > function > >>>> and the old functions that we want to deprecate / remove right? The > >> > >> Yes, the conflict is just fort the existing `fetch()` methods for which > >> we want to change the return type. > >> > >> IMHO, we should not make a breaking change in a minor release. Thus, we > >> could either only deprecate those fetch methods that return > >> `WindowStoreIterator` and do a "clean cut" in 3.0. > >> > >> Or we, follow the renaming path. No get a clean renaming, we need to > >> consider all methods that are called "fetch": > >> > >> ReadOnlyWindowStore: > >> > >>> V fetch(K, long) > >>> WindowStoreIterator fetch(K, Instant, Instant) > >>> KeyValueIterator, V> fetch(K, K, Instant, Instant) > >> > >> WindowStore: > >> > >>> WindowStoreIterator fetch(K, long, long) > >>> WindowStoreIterator fetch(K, Instant, Instant)> > >> KeyValueIterator, V> fetch(K, K, long, long) > >>> KeyValueIterator, V> fetch(K, K, Instant, Instant) > >> > >> There is also fetchAll(long, long) and fetchAll(Instant, Instant) that > >> fetch over all keys. > >> > >> Maybe we could rename `V fetch(K, long)` to `V get(K, long)` and rename > >> all `fetch()/fetchAll()` to `range()`? There is actually no reason to > >> have range()/rangeAll(). > >> > >> If we do this, we might consider to rename methods for SessionStore, > >> too. There is > >> > >>> ReadOnlySessionStore#fetch(K) > >>> ReadOnlySessionStore#fetch(K, K) > >>> SessionStore#findSessions(K, long, long) > >>> SessionStore#findSessions(K, K, long, long) > >>> SessionStore#fetchSession(K, long, long); > >> > >> Consistent renaming might be: > >> > >> ReadOnlySessionStore#range(K) > >> ReadOnlySessionStore#range(K, K) > >> SessionStore#range(K, long, long) > >> SessionStore#range(K, K, long, long) > >> SessionStore#get(K, long, long); > >> > >> > >> Not sure if extensive renaming might have too big of an impact. However, > >> `range()` and `get()` might actually be better names than `fetch()`, > >> thus, it might also provide some additional value. > >> > >> Thoughts? > >> > >> > >> -Matthias > >> > >> > >> On 3/27/19 10:19 AM, Guozhang Wang wrote: > >>> Hello Matthias, > >>> > >>> Just to clarify the naming conflicts is between the newly added > function > >>> and the old functions that we want to deprecate / remove right? The > >>> existing ones have different signatures with parameters so that they > >> should > >>> not have conflicts. > >>> > >>> I was thinking about just make the change directly without deprecating > >>> existing ones which would require users of 2.3 to make code changes -- > >> this > >>> code change looks reasonably straight-forward to me and not much worth > >>> deferring to later when the deprecated ones are removed. > >>> > >>> On the other hand, just deprecating "WindowIterator" without add new > >>> functions seems not very useful for users either since it is only used > as > >>> an indicator but users cannot make code changes during this phase > >> anyways, > >>> so it is still a `one-cut` deal when we eventually remove the > deprecated > >>> ones and add the new one. > >>> > >>> Hence I'm slightly inclining to trade compatibility and replace it with > >> new > >>> functions in one release, but if people have a good idea of the > renaming > >>> approach (I do not have a good one on top of my head :) I can also be > >>> convinced that way. > >>> > >>> Guozhang > >>> > >>> > >>> On Mon, Mar 11, 2019 at 10:41 AM Matthias J. Sax < > matthias@confluent.io> > >>> wrote: > >>> > >>>> I am open to change the return type to > >>>> > >>>> KeyValueIterator, V> > >>>> > >>>> However, this requires to rename > >>>> > >>>> #fetch(K key, long startTimestamp, long endTimestamp) > >>>> #fetch(K key, Instant startTimestamp, Instant endTimestamp) > >>>> > >>>> to avoid naming conflicts. > >>>> > >>>> What new name would you suggest? The existing methods are called > >>>> `fetch()`, `fetchAll()`, `all()`, `put()`. > >>>> > >>>> While I think it would be good to get fully aligned return types, I am > >>>> not sure how we can get aligned method names (without renaming all of > >>>> them...)? If we think it's worth to rename all to get this cleaned > up, I > >>>> am no opposed. > >>>> > >>>> > >>>> Thoughts? > >>>> > >>>> > >>>> -Matthias > >>>> > >>>> > >>>> On 3/11/19 10:27 AM, Guozhang Wang wrote: > >>>>> I was thinking about changing the return type even, to > >>>>> `KeyValueIterator, V>` since it is confusing to users > about > >>>> the > >>>>> key typed `Long` (Streams javadoc today did not explain it clearly > >>>> either), > >>>>> note it is not backward compatible at all. > >>>>> > >>>>> Personally I'd prefer to just deprecate the API and new new ones that > >>>>> return `KeyValueIterator, V>` directly, but if most > people > >>>> felt > >>>>> it is too intrusive for compatibility I can be convinced with > >>>>> `KeyValueIterator` as well. > >>>>> > >>>>> Guozhang > >>>>> > >>>>> On Mon, Mar 11, 2019 at 10:17 AM Sophie Blee-Goldman < > >>>> sophie@confluent.io> > >>>>> wrote: > >>>>> > >>>>>> I remember thinking this while working on window stores, am > definitely > >>>> for > >>>>>> it. > >>>>>> > >>>>>> On Mon, Mar 11, 2019 at 9:20 AM John Roesler > >> wrote: > >>>>>> > >>>>>>> Sounds great to me. Thanks, Matthias! > >>>>>>> -John > >>>>>>> > >>>>>>> On Sun, Mar 10, 2019 at 11:58 PM Matthias J. Sax < > >>>> matthias@confluent.io> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> Hi, > >>>>>>>> > >>>>>>>> I would like to propose KIP-439 to deprecate interface > >>>>>>>> `WindowStoreIterator`. > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-439%3A+Deprecate+Interface+WindowStoreIterator > >>>>>>>> > >>>>>>>> Looking forward to your feedback. > >>>>>>>> > >>>>>>>> > >>>>>>>> -Matthias > >>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>>> > >>>> > >>>> > >>> > >> > >> > > > > -- -- Guozhang --0000000000000c676305864ad4ea--