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