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-ign...@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.wei...@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 <ptupit...@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.wei...@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 <dsetrak...@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 <ag...@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.kasnach...@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.wei...@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/ > >>>>>> > >>>> > >> > >> > >
