Thanks, Sergey! Looks good to me. -Val
On Tue, Dec 15, 2020 at 12:12 AM Sergey Chugunov <sergey.chugu...@gmail.com> wrote: > Val, > > Your comments make total sense to me, I've fixed them and updated pull > request. Please take a look at my code when you have time. > > I also added a port range configuration to enable starting multiple > instances of ignite without specifying port manually for each instance. > > -- > Best Regards, > Sergey Chugunov > > On Sat, Dec 12, 2020 at 3:20 AM Valentin Kulichenko < > valentin.kuliche...@gmail.com> wrote: > > > Hi Sergey, > > > > Thanks for doing this. > > > > It looks like PR #5 is already under review, so I guess it will be merged > > soon. I would really love to see that, because the configuration > framework > > is one of the foundational components - we need it to continue building > > Ignite 3.0. > > > > As for PR #6, it looks a little raw, but I believe we need it to connect > > the configuration framework with the CLI tool that is also pending for > the > > merge, is this correct? If that's the case, I think it's OK to merge this > > code as a separate module, with an understanding that it will change > > significantly down the road. I would do a couple of changes though: > > > > 1. Get rid of "simplistic-ignite" naming, as it's a little confusing. > > Even though it's more of a prototype at this point, it should be clear > > what > > the module is responsible for. Can we rename it to "ignite-runner" or > > something along those lines? > > 2. Update the output - I don't like that it prints out the > > Javalin's banner and messages. I suggest replacing this with some very > > basic Ignite logging: an entry showing the version of Ignite; an entry > > indicating that the REST protocol is enabled on a certain port; an > entry > > that the process is successfully started. This is just to make sure > that > > anyone who plays with it understands what's going on. > > > > Any objections? > > > > -Val > > > > On Fri, Dec 11, 2020 at 9:53 AM Sergey Chugunov < > sergey.chugu...@gmail.com > > > > > wrote: > > > > > Hello Igniters, > > > > > > I would like to present two pull requests [1], [2] with basic > > > implementation of IEP-55 for Unified Configuration [3] and IEP-63 REST > > API > > > for Unified Configuration [4]. > > > > > > The main goal of these PRs is to present and discuss a new approach for > > > preparing and managing Ignite configuration in a more robust and > > convenient > > > way than it was before. > > > > > > These PRs cover basic aspects of configuration but other steps for > > > developing functionality are already defined; ticket IGNITE-13511 [5] > > > summarizes work to do. > > > > > > In a nutshell proposed approach to configuration is as follows: > > > > > > We want to declare configuration with POJO-based schemas that are > concise > > > and contain all important information about validation and how > different > > > pieces of configuration relate to each other. > > > When schemas are marked with annotations annotation processor enters > the > > > game and generates most of boilerplate code thus freeing users from > > writing > > > it by hand. > > > > > > REST API module from [2] contains an example of managing configuration > > and > > > exposing it to external tools like a Unified CLI tool presented in [6]. > > > > > > [1] https://github.com/apache/ignite-3/pull/5 > > > [2] https://github.com/apache/ignite-3/pull/6 > > > [3] > > > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/IEP-55+Unified+Configuration > > > [4] > > > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/IEP-63%3A+REST+API+module+to+integrate+new+modular+architecture+and+management > > > [5] https://issues.apache.org/jira/browse/IGNITE-13511 > > > [6] > > > > > > > > > http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSSION-Unified-CLI-tool-td50618.html > > > > > >
