ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Petr Ivanov <mr.wei...@gmail.com>
Subject Re: IGNITE-7107 Apache Ignite RPM packages
Date Thu, 28 Dec 2017 12:54:12 GMT
Fixed remarks and nitpicks.

Also, I purpose to delay service autostart implementation until we’ll get some first-hand
usability feedback.
Added notes to DEVNOTES.txt about how to start service.


> On 28 Dec 2017, at 14:32, Ilya Kasnacheev <ilya.kasnacheev@gmail.com> wrote:
> 
> Hello once more!
> 
> Two more nitpicks:
> 
> right now we install /usr/bin/ignitevisorcmd alternative, but it does't
> work since a) visor wants to write to /usr/share, and b) we moved it to
> examples for that.
> Please remove that alternative for now, create a ticket.
> 
> rpm -ql apache-ignite | grep \\.java
> some yardstick and ml sources are there. I don't know who is there to blame
> - Ignite release process likely, not RPM building - but they shouldn't be
> here I guess?
> 
> Regards!
> 
> -- 
> Ilya Kasnacheev
> 
> 2017-12-28 14:21 GMT+03:00 Ilya Kasnacheev <ilya.kasnacheev@gmail.com>:
> 
>> Hello again!
>> 
>> I have reviewed the modified patch. All the things in my critical list
>> were fixed. I have just two minor remarks:
>> 
>> - Please move sqlline.sh back from examples to /usr/share/a-i/bin. It
>> works just as well from there as it was from our distribution. visor
>> doesn't, hence it stays in examples.
>> - I'm a bit confused that we no longer auto-start service after package is
>> installed. You have to do
>> sudo systemctl start apache-ignite@default-config.xml
>> manually. I'm used to packages that start the thing they install
>> immediately. Of course we can just document that clearly on downloads pages
>> so people won't get lost. What do you all think? Should Ignite auto-start
>> with default config? Is it possible to do so, but make possible to turn it
>> off.
>> 
>> So from my side I recommend this pull request for inclusion after 1) is
>> done and 2) is discussed.
>> 
>> Also, would be nice if you create additional JIRA tickets for issues in my
>> minor list (ignitesqlline in /usr/bin, ignitevisorcmd in /usr/bin and
>> working) to be implemented in future releases.
>> 
>> Regards,
>> 
>> --
>> Ilya Kasnacheev
>> 
>> 2017-12-26 15:25 GMT+03:00 Petr Ivanov <mr.weider@gmail.com>:
>> 
>>> Removed replacement for default-config.xml.
>>> Also reimplemented service to be able to run multiple instances of Ignite.
>>> 
>>> See updated PR [1].
>>> 
>>> 
>>> [1] https://github.com/apache/ignite/pull/3171 <
>>> https://github.com/apache/ignite/pull/3171>
>>> 
>>> 
>>> 
>>>> On 25 Dec 2017, at 18:57, Pavel Tupitsyn <ptupitsyn@apache.org> wrote:
>>>> 
>>>> PDS and discovery settings are unrelated to the packaging task.
>>>> Andrey is right, just use existing default config.
>>>> 
>>>> On Mon, Dec 25, 2017 at 6:51 PM, Petr Ivanov <mr.weider@gmail.com>
>>> wrote:
>>>> 
>>>>> Can we change default configuration file then?
>>>>> So that both binary and package deliveries contained PDS turned on by
>>>>> default?
>>>>> 
>>>>> And what about Multicast Discovery?
>>>>> 
>>>>> 
>>>>>> On 25 Dec 2017, at 18:41, Dmitriy Setrakyan <dsetrakyan@apache.org>
>>>>> wrote:
>>>>>> 
>>>>>> I agree with Andrey. The product should be identical regardless of
how
>>>>> you
>>>>>> download and install it.
>>>>>> 
>>>>>> On Mon, Dec 25, 2017 at 7:18 AM, Andrey Gura <agura@apache.org>
>>> wrote:
>>>>>> 
>>>>>>> Hi,
>>>>>>> 
>>>>>>> I think we should provide the same default configuration for
all
>>>>>>> binary builds. From my point of view RPM/DEB/etc packages should
use
>>>>>>> default configuration file like to standard binary release.
>>>>>>> 
>>>>>>> On Mon, Dec 25, 2017 at 4:39 PM, Ilya Kasnacheev
>>>>>>> <ilya.kasnacheev@gmail.com> wrote:
>>>>>>>> Hello Igniters!
>>>>>>>> 
>>>>>>>> What's your take on enabling PDS in 2.4 in default config?
This way
>>> we
>>>>>>>> could enable it in RPM build with easy heart.
>>>>>>>> 
>>>>>>>> The rest of your answers seem spot on. I'll happily review
your
>>> amended
>>>>>>>> patch when it is ready (later this week?)
>>>>>>>> 
>>>>>>>> --
>>>>>>>> Ilya Kasnacheev
>>>>>>>> 
>>>>>>>> 2017-12-22 18:27 GMT+03:00 vveider <mr.weider@gmail.com>:
>>>>>>>> 
>>>>>>>>> Ilya Kasnacheev wrote
>>>>>>>>>> I have noticed that both spec and DEVNOTES are made
to use tabs
>>>>>>> alongside
>>>>>>>>>> with spaces. I thought we are avoiding tabs? Please
confirm that
>>>>>>> usage of
>>>>>>>>>> tabs is desired in this case.
>>>>>>>>> 
>>>>>>>>> My fault - got used to editing such files in default
vim. Fixed and
>>>>>>> updated
>>>>>>>>> vim settings to indent with spaces.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Ilya Kasnacheev wrote
>>>>>>>>>> Maybe it's my CentOS but initially build will fail
with the
>>> following
>>>>>>>>>> message, which I had to fix in spec
>>>>>>>>>> + cd 'apache-ignite-*'
>>>>>>>>>> /var/tmp/rpm-tmp.KwOoyl: line 34: cd: apache-ignite-*:
No such
>>> file
>>>>> or
>>>>>>>>>> directory
>>>>>>>>> 
>>>>>>>>> Strange error - shell expansion is not working. Anyway,
replaced
>>> with
>>>>>>> more
>>>>>>>>> universal variant.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Ilya Kasnacheev wrote
>>>>>>>>>> We absolutely should not inline configuration files
(such as
>>>>>>>>>> default-config.xml) into build scripts. We should
just copy over
>>>>>>>>>> default-config.xml from config/ to /etc, with dependencies
if
>>> needed.
>>>>>>>>> 
>>>>>>>>> Extracted corresponding sections into separate files.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Ilya Kasnacheev wrote
>>>>>>>>>> I'm not sure PDS should be ON by default. Better
provide it in
>>>>>>> examples
>>>>>>>>>> but disable by default.
>>>>>>>>> 
>>>>>>>>> AFAIK, PDS is actively pushing forward and has to be
enabled by
>>>>> default
>>>>>>> so
>>>>>>>>> that Ignite users start working with PDS from the very
beginning.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Ilya Kasnacheev wrote
>>>>>>>>>> I cannot comment on firewall rules validity, but
firewall and
>>> service
>>>>>>>>>> configurations should be stored in sources, as files,
supplied
>>> with
>>>>>>>>>> release (somewhere in packages/) and installed from
there
>>>>>>>>> 
>>>>>>>>> Unfortunately, I did not find any other way to open ports
and
>>>>> multicast,
>>>>>>>>> except applying direct iptables rules though firewall-cmd
-- so
>>> there
>>>>>>> can
>>>>>>>>> be
>>>>>>>>> no separate service firewall rules file (as can be found
here:
>>>>>>>>> /usr/lib/firewalld/services). Applied rules from package
go
>>> straight
>>>>> to
>>>>>>>>> /etc/firewalld/direct.xml. If we agree not to put
>>> Multicast-enabled by
>>>>>>>>> default configuration file and will stick to IP Discovery,
I'll
>>> try to
>>>>>>>>> revise firewall configuration and create separate service
firewall
>>>>> rules
>>>>>>>>> file (but, I guess, much later).
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Ilya Kasnacheev wrote
>>>>>>>>>> * sqlline.sh fails to connect initially, needs to
be specified
>>>>>>>>>> jdbc:ignite:thin://localhost, then it works. I think
that sqlline
>>>>>>> should
>>>>>>>>>> become available as /usr/bin/apache-ignite-sqlline
for example,
>>> to be
>>>>>>> in
>>>>>>>>>> $PATH. And figure out how to connect by itself.
>>>>>>>>>> * ignitevisorcmd.sh becomes confused. first it scans
>>>>>>>>>> /usr/share/apache-ignite for configs, then it fails
to write to
>>>>>>>>>> /usr/share/apache-ignite/work. We should consider
how it can be
>>> fixed
>>>>>>> (a
>>>>>>>>>> symlink from /usr/share/apache-ignite/config to
>>> /etc/apache-ignite,
>>>>>>>>>> perhaps, and work dir in /tmp?), and made available
as
>>>>>>>>>> /usr/bin/apache-ignite-visorcmd.
>>>>>>>>> 
>>>>>>>>> The problem exists mainly because of sqlline.sh|ignitevisorcmd.sh
>>>>>>>>> unaware-of-package design and I see the following 3-step
way to
>>>>> overcome
>>>>>>>>> this limitation.
>>>>>>>>> At first we hide this executables file in examples (As
Iliya
>>>>> proposed).
>>>>>>>>> Secondly, I can design a patch, which can be applied
during package
>>>>>>> build,
>>>>>>>>> so that those executables work normally form the package
>>> installation.
>>>>>>>>> And lastly, redesign them to be universal and compatible
with all
>>>>>>> delivery
>>>>>>>>> methods.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Ilya Kasnacheev wrote
>>>>>>>>>> Not everything should go into /usr/share. classpath
libs belong to
>>>>>>>>>> /usr/lib.
>>>>>>>>> 
>>>>>>>>> Moved libs to /usr/lib/apache-ignite and mapped to
>>>>>>>>> /usr/share/apache-ignite/libs (for backward compatibility).
I guess
>>>>>>> later
>>>>>>>>> we
>>>>>>>>> have to think out of a solution to load libs directly
from /usr/lib
>>>>>>> (along
>>>>>>>>> with configurations / logs / work / etc.)
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Ilya Kasnacheev wrote
>>>>>>>>>> We are building from binary package and not from
sources. If it is
>>>>>>> used
>>>>>>>>> by
>>>>>>>>>> internal RPM building this is tolerable, but any
distro or
>>> repository
>>>>>>>>> will
>>>>>>>>>> want to build from source.
>>>>>>>>> 
>>>>>>>>> It's not an ordinary task to create "100% honest" package,
so I
>>> guess
>>>>> we
>>>>>>>>> definitely won't make to 2.4 release. So I purpose include
in the
>>>>>>> nearest
>>>>>>>>> release our package built from binary archive and do
not supply
>>>>> sources
>>>>>>>>> package (as it is currently done in apache.org/dist for
cassandra
>>> /
>>>>>>> hadoop
>>>>>>>>> or alike). And later design package build procedure,
which includes
>>>>>>> getting
>>>>>>>>> sources from sources distribution, or even from Apache
Ignite Git
>>>>>>>>> repository
>>>>>>>>> tag.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Ilya Kasnacheev wrote
>>>>>>>>>> spec contains version (2.4.0) - will be needed to
be changed for
>>>>> every
>>>>>>>>>> release. I'm not sure if it's possible to extract.
>>>>>>>>> 
>>>>>>>>> Release procedure on TC is already has some task for
project
>>> version
>>>>>>>>> update,
>>>>>>>>> so I guess it is not so difficult to update it accordingly
(as it
>>> is
>>>>>>>>> currently done for C++ / .Net internal versions).
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Ilya Kasnacheev wrote
>>>>>>>>>> Package description may be improved. service description
lacks
>>> '-' in
>>>>>>> "In
>>>>>>>>>> Memory".
>>>>>>>>> 
>>>>>>>>> Fixed.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> So the only question to discuss (according Iliya's review)
is
>>> whether
>>>>> to
>>>>>>>>> supply package with default-config.xml that has Multicast
Discovery
>>>>>>> turned
>>>>>>>>> on or not?
>>>>>>>>> 
>>>>>>>>> Also I'll appreciate any other comments concerning current
package
>>>>>>> design.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> --
>>>>>>>>> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>>>> 
>>> 
>>> 
>> 


Mime
View raw message