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.kasnach...@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-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/ >> >>>>>> >> >>>> >> >> >> >> >> >> >
