Re: [DISCUSS] Move non-connectors modules to out of external

[Jungtaek Lim <ka...@gmail.com>]

UPDATES: I've filed an issue STORM-2453 [1] and submitted a PR [2].
Please review so that we can finalize this discussion.

Thanks,
Jungtaek Lim (HeartSaVioR)

[1] https://issues.apache.org/jira/browse/STORM-2453
[2] https://github.com/apache/storm/pull/2042

2017년 3월 31일 (금) 오후 3:40, Xin Wang <da...@gmail.com>님이 작성:

Jungtaek,

Currently no patches, the work is in progress. I will submit several days
later.

2017-03-31 14:18 GMT+08:00 Jungtaek Lim <ka...@gmail.com>:

> Xin,
>
> Do you already have patches? Or just waiting for this before doing some
> works?
>
> 2017년 3월 31일 (금) 오후 3:10, Xin Wang <da...@gmail.com>님이 작성:
>
> > +1 on keeping the current external/storm-* in place and move flux, sql,
> > storm-submit-tools into top-level.
> >
> > BTW, I am waiting for the discuss result before submitting several SQL
> > related PRs. :)
> >
> > - Xin
> >
> > 2017-03-31 10:41 GMT+08:00 Jungtaek Lim <ka...@gmail.com>:
> >
> > > Yeah sure I'm OK to just apply it for master branch.
> > > Are you okay for moving them to root directory without renaming? Or do
> > you
> > > want to rename or suggest other base directory?
> > >
> > > 2017년 3월 31일 (금) 오전 11:36, P. Taylor Goetz <pt...@gmail.com>님이 작성:
> > >
> > > > I'm hesitant to change the layout of the 1.x release. People do some
> > > crazy
> > > > things when it comes to operations that we can't predict. I'm okay
> with
> > > > doing this on the master/2.0 branch, but I'm hesitant on the 1.x
> > branch.
> > > >
> > > > -Taylor
> > > >
> > > > > On Mar 30, 2017, at 9:01 PM, Jungtaek Lim <ka...@gmail.com>
> wrote:
> > > > >
> > > > > Once we just released Storm 1.1.0, I guess we can continue
> discussion
> > > > here.
> > > > >
> > > > > I checked the PR list, and there're no open PRs for among Flux,
> Storm
> > > > SQL,
> > > > > and Storm submit tool. So it's good to go.
> > > > >
> > > > > I think we have consensus to move non-connectors modules to out of
> > > > > external. There're also some interest about renaming "external" to
> > > > > "connectors", but given that "external" is chosen by community and
> > has
> > > > been
> > > > > the norm for years, so I agree it would be better not to rename
it.
> > We
> > > > can
> > > > > do it later when there's another chance of doing it.
> > > > >
> > > > > We didn't decide where to move, but most of us seems to be OK to
> move
> > > to
> > > > > the top directory.
> > > > > Taylor, any further opinion regarding this?
> > > > > Suppose we're moving them to the top directory (or whatever the
> same
> > > base
> > > > > directory), what would be good names for them? Flux doesn't have
> > prefix
> > > > > 'storm' so a bit different, but if we're OK we skip renaming it.
> > > > >
> > > > > I'll do the work when Taylor is OK for changing this.
> > > > >
> > > > > - Jungtaek Lim (HeartSaVioR)
> > > > >
> > > > > 2017년 3월 27일 (월) 오전 11:16, Jungtaek Lim <ka...@gmail.com>님이 작성:
> > > > >
> > > > >> 1. apply versions
> > > > >>
> > > > >> First plan was applying this only for master, but realized that
> > > > >> contributors should make two patches for every PRs when we apply
> > this
> > > to
> > > > >> only master.
> > > > >>
> > > > >> So in order to make less inconvenience, it would be better to
> apply
> > > this
> > > > >> for both 1.x and master. It also affects opened pull requests so
> we
> > > > would
> > > > >> like to check that relevant PRs are open, and apply it later than
> > > > reviewing
> > > > >> them.
> > > > >>
> > > > >> I agree with Harsha. No need to make change for current release
> > vote.
> > > > >>
> > > > >> 2. naming issue for "external"
> > > > >>
> > > > >> "external" makes me feel that it's related to "external"
> component,
> > > say,
> > > > >> outside of Storm. That's why I suggest moving non-connectors to
> out
> > of
> > > > >> "external". IMHO "connector" is still more intuitive and
> > > > self-describing,
> > > > >> but I understand that renaming the directory structure would be
> > > painful,
> > > > >> and "storm-kafka-monitor" is an example of what it's not a
> > "connector"
> > > > but
> > > > >> an "external". So I'm OK to keep it as "external".
> > > > >>
> > > > >> 3. where to move non-connectors
> > > > >>
> > > > >> Except Flux they're directly supported by Storm. I mean
"storm.py"
> > is
> > > > >> aware of them and supports them, so for me they are eligible to
> move
> > > to
> > > > the
> > > > >> top directory. I'm open to other suggestion as well.
> > > > >>
> > > > >> - Jungtaek Lim (HeartSaVioR)
> > > > >>
> > > > >> 2017년 3월 26일 (일) 오후 12:14, Harsha Chintalapani <storm@harsha.io
> >님이
> > > 작성:
> > > > >>
> > > > >> We should this in next release of 1.x or 2.0. I am +1 on continue
> > with
> > > > >> current release.
> > > > >> -Harsha
> > > > >>> On Fri, Mar 24, 2017 at 8:53 PM P. Taylor Goetz <
> ptgoetz@gmail.com
> > >
> > > > wrote:
> > > > >>>
> > > > >>> The question remains if we want to do this in the 1.1.0 release,
> or
> > > > >> later.
> > > > >>>
> > > > >>> If it's the 1.1.0 release we need to make the changes and cut
> > another
> > > > RC.
> > > > >>> I'm fine with that, but want to make sure we have consensus
> before
> > > > going
> > > > >>> down that road.
> > > > >>>
> > > > >>> -Taylor
> > > > >>>
> > > > >>>> On Mar 24, 2017, at 10:57 PM, Harsha Chintalapani <
> > storm@harsha.io>
> > > > >>> wrote:
> > > > >>>>
> > > > >>>> Agree on change like this would be confusing to the users. Lets
> > keep
> > > > >> the
> > > > >>>> original plan of moving non-connectors modules of external
> instead
> > > of
> > > > >>>> introducing new changes
> > > > >>>> that are not in scope of this discussion.
> > > > >>>> My +1 still stands on keeping the current external/storm-* in
> > place
> > > > and
> > > > >>>> move just sql and storm-perf into top-level. We can have
> > discussion
> > > > for
> > > > >>>> storm 2.0 if we want to do
> > > > >>>> more changes.
> > > > >>>>
> > > > >>>> -Harsha
> > > > >>>>
> > > > >>>>> On Fri, Mar 24, 2017 at 4:31 PM P. Taylor Goetz <
> > ptgoetz@gmail.com
> > > >
> > > > >>> wrote:
> > > > >>>>>
> > > > >>>>> If we decide to change the structure of the distribution like
> > > this, I
> > > > >>>>> think we should do it in masrwe/2.0. If we want this for 1.1.0
> we
> > > > need
> > > > >>> to
> > > > >>>>> cut a new release candidate.
> > > > >>>>>
> > > > >>>>> Changing the structure of the distribution file structure can
> be
> > > > >>>>> disruptive for users. Even the change to no longer include
> > > connector
> > > > >>>>> binaries, as we've learned, will be a headache for some users.
> > > > >>>>>
> > > > >>>>> IMHO, from an ops perspective, changes like this should be
> > handled
> > > > >> like
> > > > >>>>> API changes.
> > > > >>>>>
> > > > >>>>> -Taylor
> > > > >>>>>
> > > > >>>>>> On Mar 24, 2017, at 4:07 PM, Hugo Da Cruz Louro <
> > > > >>> hlouro@hortonworks.com>
> > > > >>>>> wrote:
> > > > >>>>>>
> > > > >>>>>> Another possibility is to keep the ‘external' module, and
> create
> > > sub
> > > > >>>>> modules under it. The legacy structure would remain intact,
> while
> > > > >> making
> > > > >>>>> things more modular. An idea would be:
> > > > >>>>>>
> > > > >>>>>> + external
> > > > >>>>>>   + connectors
> > > > >>>>>>   + tools
> > > > >>>>>>   + monitoring
> > > > >>>>>>   + etc
> > > > >>>>>>
> > > > >>>>>> Hugo
> > > > >>>>>>
> > > > >>>>>>> On Mar 24, 2017, at 12:34 PM, P. Taylor Goetz <
> > ptgoetz@gmail.com
> > > >
> > > > >>>>> wrote:
> > > > >>>>>>>
> > > > >>>>>>> For the background on why “external” was selected, you have
> to
> > go
> > > > >> back
> > > > >>>>> to a lengthy discussion in Feb. 2014.
> > > > >>>>>>>
> > > > >>>>>>> Here’s the start of the thread:
> > > > >>>>>>>
> > > > >>>>>>>
> > > > >>>>>
> > > > >>>
> > > > >>
> > > > http://mail-archives.apache.org/mod_mbox/storm-dev/201402.
> > > mbox/%3cEE2BD0E2-254C-47AF-8A53-257DB7F05A7A@gmail.com%3e
> > > > >>>>> <
> > > > >>>>>
> > > > >>>
> > > > >>
> > > > http://mail-archives.apache.org/mod_mbox/storm-dev/201402.
> > > mbox/%3CEE2BD0E2-254C-47AF-8A53-257DB7F05A7A@gmail.com%3E
> > > > >>>>>>
> > > > >>>>>>>
> > > > >>>>>>> It continues into March:
> > > > >>>>>>>
> > > > >>>>>>>
> > > > >>>>>
> > > > >>>
> > > > >>
> > > > http://mail-archives.apache.org/mod_mbox/storm-dev/201403.mbox/%
> > > 3cCADiMvzUm1d3oM30ZAYqq4xXE1VjbN7fUMQcsgU+524OQgecSaw@mail.gmail.com
> %3e
> > > > >>>>>>>
> > > > >>>>>>> I’m -1 on renaming “external”. That’s the name chosen by the
> > > > >> community
> > > > >>>>> and it has been the norm for 3 years. Changing it would likely
> > > > confuse
> > > > >>>>> users.
> > > > >>>>>>>
> > > > >>>>>>> One of the ideas behind “external” was that it would contain
> > > > >>> components
> > > > >>>>> that were not essential to running storm. That line has
> recently
> > > > >> blurred
> > > > >>>>> with some non-connector code sneaking in, so I’m okay with
> moving
> > > > >>>>> non-connector code out of external. Another point in that
> thread
> > > was
> > > > a
> > > > >>>>> desire to avoid cluttering up the root directory, so we need
to
> > be
> > > > >>> careful
> > > > >>>>> about what the destination for those components is.
> > > > >>>>>>>
> > > > >>>>>>> -Taylor
> > > > >>>>>>>
> > > > >>>>>>>> On Mar 24, 2017, at 3:11 PM, Hugo Da Cruz Louro <
> > > > >>>>> hlouro@hortonworks.com> wrote:
> > > > >>>>>>>>
> > > > >>>>>>>> +1 non-connectors to top level
> > > > >>>>>>>> +1 to renaming external to connectors
> > > > >>>>>>>>
> > > > >>>>>>>> As for storm-kaka, if we are already touching the external
> > > > modules,
> > > > >>>>> all the modules should be a submodule of a parent module
called
> > > > >>>>> storm-kafka. I don’t think we should have 3 parent modules as
> we
> > > > >>> currently
> > > > >>>>> have (storm-kafka, storm-kafka-client, storm-kafka-monitor)
> > > > >>>>>>>>
> > > > >>>>>>>> The structure should be something along the lines (I don’t
> > mean
> > > > the
> > > > >>>>> exact names;  we should find better ones. storm-kafka and
> > > > >>>>> storm-kafka-client are not very self explanatory in my
opinion)
> > > > >>>>>>>>
> > > > >>>>>>>> + storm-kafka
> > > > >>>>>>>> + monitoring
> > > > >>>>>>>> + new-client
> > > > >>>>>>>> + old-client
> > > > >>>>>>>>
> > > > >>>>>>>> If we have to create new modules or submodules (e.g. under
> > > utils)
> > > > >> so
> > > > >>>>> be it. The code should be in a module that is named after what
> > its
> > > > >>> doing.
> > > > >>>>>>>>
> > > > >>>>>>>> Hugo
> > > > >>>>>>>>
> > > > >>>>>>>>> On Mar 24, 2017, at 11:15 AM, Priyank Shah <
> > > > pshah@hortonworks.com
> > > > >>>
> > > > >>>>> wrote:
> > > > >>>>>>>>>
> > > > >>>>>>>>> +1 to moving non-conncectors to top level. I think we
> should
> > > keep
> > > > >>>>> stom-kafka-monitor under external or connectors(after
> renaming).
> > > > >>>>>>>>>
> > > > >>>>>>>>> Jungtaek, just to clarify on what you said regarding storm
> > core
> > > > >>>>> referencing storm-kafka-monitor. Like you said its just
calling
> > the
> > > > >>> script
> > > > >>>>> from ui jvm. There is no dependency in terms of class files
> > needed
> > > to
> > > > >>> run
> > > > >>>>> the script from ui. The script itself adds a –cp argument and
> all
> > > it
> > > > >>> needs
> > > > >>>>> is storm-kafka-monitor jar in classpath. As far as packaging
> the
> > > > >> script
> > > > >>> is
> > > > >>>>> concerned we can do what Satish suggested. i.e. move it to
> > > > >>>>> storm-kafka-monitor in source and while packaging put it under
> > bin.
> > > > >>>>> Reiterating to make sure I am not mis-understanding anything.
> > > > >>>>>>>>>
> > > > >>>>>>>>> On 3/24/17, 9:14 AM, "Harsha Chintalapani" <
> storm@harsha.io>
> > > > >> wrote:
> > > > >>>>>>>>>
> > > > >>>>>>>>> +1 on moving non-connectors to top-level like sql and
> > > storm-perf.
> > > > >>>>>>>>> Regarding storm-kafka-monitor we can move this into "util"
> > > folder
> > > > >> or
> > > > >>>>> keep
> > > > >>>>>>>>> in the external.
> > > > >>>>>>>>> -Harsha
> > > > >>>>>>>>>
> > > > >>>>>>>>> On Fri, Mar 24, 2017 at 2:23 AM Satish Duggana <
> > > > >>>>> satish.duggana@gmail.com>
> > > > >>>>>>>>> wrote:
> > > > >>>>>>>>>
> > > > >>>>>>>>>> storm-kafka-monitor is not a connector by itself but it
is
> > > > >> related
> > > > >>>>> to kafka
> > > > >>>>>>>>>> connectors. So, any utility related to that connector
> should
> > > be
> > > > >>> part
> > > > >>>>> of
> > > > >>>>>>>>>> that connector module(can be a submodule) instead of a
top
> > > level
> > > > >>>>> module.
> > > > >>>>>>>>>> core/ui uses this utility referring directly in a hacky
> way,
> > > > >> which
> > > > >>>>> we may
> > > > >>>>>>>>>> want to fix later. storm-kafka-monitor script exists in
> bin
> > > > >>>>> directory which
> > > > >>>>>>>>>> can be moved to storm-kafka-monitor module and the same
> > script
> > > > >> can
> > > > >>> be
> > > > >>>>>>>>>> packaged as part of storm/bin directory while packaging
> the
> > > > >>>>> distribution.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> Thanks,
> > > > >>>>>>>>>> ~Satish.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>>> On Fri, Mar 24, 2017 at 1:07 PM, Jungtaek Lim <
> > > > >> kabhwan@gmail.com>
> > > > >>>>> wrote:
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> storm-kafka-monitor is referred by storm-core, though
> it's
> > > > >>>>> referenced via
> > > > >>>>>>>>>>> executing command. Yes it's a bit odd to place it as top
> > > > >>> directory,
> > > > >>>>> but
> > > > >>>>>>>>>>> it's not a connector for that reason too. Neither is
> ideal
> > > for
> > > > >> me,
> > > > >>>>> so
> > > > >>>>>>>>>>> ironically, either is fine.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> - Jungtaek Lim (HeartSaVioR)
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> 2017년 3월 24일 (금) 오후 4:19, Satish Duggana <
> > > > >>> satish.duggana@gmail.com
> > > > >>>>>> 님이
> > > > >>>>>>>>>> 작성:
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>>> +1 except for storm-kafka-monitor module as this
utility
> > is
> > > > >> more
> > > > >>>>> about
> > > > >>>>>>>>>>>> querying topic/partition offsets of kafka spouts in a
> > > > topology.
> > > > >>> Do
> > > > >>>>> not
> > > > >>>>>>>>>> we
> > > > >>>>>>>>>>>> want to push this module into connectors/kafka as a
> > > submodule
> > > > >>> along
> > > > >>>>>>>>>> with
> > > > >>>>>>>>>>>> other submodules including old/new kafka spout modules?
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> Thanks,
> > > > >>>>>>>>>>>> Satish.
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> On Fri, Mar 24, 2017 at 12:10 PM, Arun Iyer <
> > > > >>> aiyer@hortonworks.com
> > > > >>>>>>
> > > > >>>>>>>>>>> wrote:
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>>> +1
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>> Makes sense to move the non-connectors to top level
and
> > > keep
> > > > >>> only
> > > > >>>>> the
> > > > >>>>>>>>>>>>> connectors under “connectors” folder.
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> On 3/24/17, 12:00 PM, "Jungtaek Lim" <
> kabhwan@gmail.com
> > >
> > > > >>> wrote:
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> (Sent this yesterday but can't find this from
> storm-dev
> > > > >> mbox...
> > > > >>>>>>>>>>> sending
> > > > >>>>>>>>>>>> it
> > > > >>>>>>>>>>>>>> again)
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> Hi dev,
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> I'd like to start discussion regarding moving
> > > non-connectors
> > > > >>>>> modules
> > > > >>>>>>>>>>> out
> > > > >>>>>>>>>>>>> of
> > > > >>>>>>>>>>>>>> external, maybe top directory.
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> "external" directory has non-connectors (SQL, Flux,
> > > > >>>>>>>>>>> storm-kafka-monitor,
> > > > >>>>>>>>>>>>>> storm-submit-tools), and except Flux, others should
be
> > > > placed
> > > > >>> to
> > > > >>>>> the
> > > > >>>>>>>>>>>>> binary
> > > > >>>>>>>>>>>>>> dist. since Storm itself (not from user topology)
> needs
> > to
> > > > >>> refer
> > > > >>>>>>>>>> them.
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> They're actually tied to the core of Storm, so I feel
> > that
> > > > it
> > > > >>>>> would
> > > > >>>>>>>>>> be
> > > > >>>>>>>>>>>>>> better to treat them (including Flux) as
non-external,
> > > maybe
> > > > >>> same
> > > > >>>>>>>>>>> level
> > > > >>>>>>>>>>>> as
> > > > >>>>>>>>>>>>>> storm-core.
> > > > >>>>>>>>>>>>>> (I'm not sure what "external" actually means for
Storm
> > > > >> project
> > > > >>>>> btw.)
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> In addition, after doing that I'd like to change the
> > > > >> directory
> > > > >>>>> name
> > > > >>>>>>>>>>>>>> "external" to "connector" or so, so that the name
> could
> > be
> > > > >>>>>>>>>>>> self-describing
> > > > >>>>>>>>>>>>>> and we can only place connectors to that directory.
> > > > >>>>>>>>>>>>>> (I know it would be painful for already opened pull
> > > > requests,
> > > > >>> so
> > > > >>>>> no
> > > > >>>>>>>>>>>> strong
> > > > >>>>>>>>>>>>>> opinion regarding this.)
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> Looking forward to your opinion!
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> Thanks,
> > > > >>>>>>>>>>>>>> Jungtaek Lim (HeartSaVioR)
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>
> > > > >>>>>>>>>
> > > > >>>>>>>>>
> > > > >>>>>>>>
> > > > >>>>>>>
> > > > >>>>>>
> > > > >>>>>
> > > > >>>
> > > > >>
> > > > >>
> > > >
> > >
> >
>