Guys, I've merged change to master branch. Thanks a lot!
On Thu, Dec 28, 2017 at 3:54 PM, Petr Ivanov <mr.wei...@gmail.com> wrote: > 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/ >>>>>>>>>> >>>>>>>> >>>>>> >>>>>> >>>> >>>> >>> >
