I don't think that making javadocs more descriptive can be considered as harmful code base enlargement. I'd recommend to extend the docs, but the last word is yours ;)
On Tue, Mar 31, 2020 at 2:44 PM Vladimir Steshin <vlads...@gmail.com> wrote: > Ivan, hi. > > I absolutely agree this particular description is not enough to see the > deactivation issue. I also vote for brief code. > > There are about 15 places in inner logic with this description. I > propose balance between code base size and comment completeness. > > Should we enlarge code even if we got several full descriptions? > > > 30.03.2020 20:02, Ivan Rakov пишет: > > Vladimir, > > > > @param forceDeactivation If {@code true}, cluster deactivation will be > >> forced. > > It's true that it's possible to infer semantics of forced deactivation > from > > other parts of API. I just wanted to highlight that exactly this > > description explains something that can be guessed by the parameter name. > > I suppose to shorten the lookup path and shed a light on deactivation > > semantics a bit: > > > >> @param forceDeactivation If {@code true}, cluster will be deactivated > even > >> if running in-memory caches are present. All data in the corresponding > >> caches will be vanished as a result. > > Does this make sense? > > > > On Fri, Mar 27, 2020 at 12:00 PM Vladimir Steshin <vlads...@gmail.com> > > wrote: > > > >> Ivan, hi. > >> > >> > >> 1) >>> Is it correct? If we are on the same page, let's proceed this way > >> > >> It is correct. > >> > >> > >> 2) - In many places in the code I can see the following javadoc > >> > >>> @param forceDeactivation If {@code true}, cluster deactivation will > be > >> forced. > >> > >> In the internal params/flags. You can also find /@see > >> ClusterState#INACTIVE/ and full description with several public APIs ( > >> like /Ignite.active(boolean)/ ): > >> > >> // > >> > >> /* <p>/ > >> > >> // > >> > >> /* <b>NOTE:</b>/ > >> > >> // > >> > >> /* Deactivation clears in-memory caches (without persistence) including > >> the system caches./ > >> > >> Should be enough. Is not? > >> > >> > >> 27.03.2020 10:51, Ivan Rakov пишет: > >>> Vladimir, Igniters, > >>> > >>> Let's emphasize our final plan. > >>> > >>> We are going to add --force flags that will be necessary to pass for a > >>> deactivation if there are in-memory caches to: > >>> 1) Rest API (already implemented in [1]) > >>> 2) Command line utility (already implemented in [1]) > >>> 3) JMX bean (going to be implemented in [2]) > >>> We are *not* going to change IgniteCluster or any other thick Java API, > >>> thus we are *not* going to merge [3]. > >>> We plan to *fully rollback* [1] and [2] once cache data survival after > >>> activation-deactivation cycle will be implemented. > >>> > >>> Is it correct? If we are on the same page, let's proceed this way. > >>> I propose to: > >>> - Create a JIRA issue for in-memory-data-safe deactivation (possibly, > >>> without IEP and detailed design so far) > >>> - Describe in the issue description what exact parts of API should be > >>> removed under the issue scope. > >>> > >>> Also, a few questions on already merged [1]: > >>> - We have removed GridClientClusterState#state(ClusterState) from Java > >>> client API. Is it a legitimate thing to do? Don't we have to support > API > >>> compatibility for thin clients as well? > >>> - In many places in the code I can see the following javadoc > >>> > >>>> @param forceDeactivation If {@code true}, cluster deactivation will > >> be forced. > >>>> As for me, this javadoc doesn't clarify anything. I'd suggest to > >> describe > >>> in which cases deactivation won't happen unless it's forced and which > >>> impact forced deactivation will bring on the system. > >>> > >>> [1]: https://issues.apache.org/jira/browse/IGNITE-12701 > >>> [2]: https://issues.apache.org/jira/browse/IGNITE-12779 > >>> [3]: https://issues.apache.org/jira/browse/IGNITE-12614 > >>> > >>> -- > >>> Ivan > >>> > >>> On Tue, Mar 24, 2020 at 7:18 PM Vladimir Steshin <vlads...@gmail.com> > >> wrote: > >>>> Hi, Igniters. > >>>> > >>>> I'd like to remind you that cluster can be deactivated by user with 3 > >>>> utilities: control.sh, *JMX and the REST*. Proposed in [1] solution is > >>>> not about control.sh. It suggests same approach regardless of the > >>>> utility user executes. The task touches *only* *API of the user > calls*, > >>>> not the internal APIs. > >>>> > >>>> The reasons why flag “--yes” and confirmation prompt hasn’t taken into > >>>> account for control.sh are: > >>>> > >>>> -Various commands widely use “--yes” just to start. Even not dangerous > >>>> ones require “--yes” to begin. “--force” is dedicated for *harmless > >>>> actions*. > >>>> > >>>> -Checking of probable data erasure works after command start and > >>>> “--force” may not be required at all. > >>>> > >>>> -There are also JMX and REST. They have no “—yes” but should work > alike. > >>>> > >>>> To get the deactivation safe I propose to merge last ticket > with > >>>> the JMX fixes [2]. In future releases, I believe, we should estimate > >>>> jobs and fix memory erasure in general. For now, let’s prevent it. > WDYT? > >>>> > >>>> > >>>> [1] https://issues.apache.org/jira/browse/IGNITE-12614 > >>>> > >>>> [2] https://issues.apache.org/jira/browse/IGNITE-12779 > >>>> > >>>> > >>>> 24.03.2020 15:55, Вячеслав Коптилин пишет: > >>>>> Hello Nikolay, > >>>>> > >>>>> I am talking about the interactive mode of the control utility, which > >>>>> requires explicit confirmation from the user. > >>>>> Please take a look at DeactivateCommand#prepareConfirmation and its > >>>> usages. > >>>>> It seems to me, this mode has the same aim as the forceDeactivation > >> flag. > >>>>> We can change the message returned by > >>>> DeactivateCommand#confirmationPrompt > >>>>> as follows: > >>>>> "Warning: the command will deactivate the cluster nnn and > clear > >>>>> in-memory caches (without persistence) including system caches." > >>>>> > >>>>> What do you think? > >>>>> > >>>>> Thanks, > >>>>> S. > >>>>> > >>>>> вт, 24 мар. 2020 г. в 13:07, Nikolay Izhikov <nizhi...@apache.org>: > >>>>> > >>>>>> Hello, Slava. > >>>>>> > >>>>>> Are you talking about this commit [1] (sorry for commit message it’s > >> due > >>>>>> to the Github issue)? > >>>>>> > >>>>>> The message for this command for now > >>>>>> > >>>>>> «Deactivation stopped. Deactivation clears in-memory caches (without > >>>>>> persistence) including the system caches.» > >>>>>> > >>>>>> Is it clear enough? > >>>>>> > >>>>>> [1] > >>>>>> > >> > https://github.com/apache/ignite/commit/4921fcf1fecbd8a1ab02099e09cc2adb0b3ff88a > >>>>>>> 24 марта 2020 г., в 13:02, Вячеслав Коптилин < > >> slava.kopti...@gmail.com > >>>>>> написал(а): > >>>>>>> Hi Nikolay, > >>>>>>> > >>>>>>>> 1. We should add —force flag to the command.sh deactivation > command. > >>>>>>> I just checked and it seems that the deactivation command > >>>>>>> (control-utility.sh) already has a confirmation option. > >>>>>>> Perhaps, we need to clearly state the consequences of using this > >>>> command > >>>>>>> with in-memory caches. > >>>>>>> > >>>>>>> Thanks, > >>>>>>> S. > >>>>>>> > >>>>>>> вт, 24 мар. 2020 г. в 12:51, Nikolay Izhikov <nizhi...@apache.org > >: > >>>>>>> > >>>>>>>> Hello, Alexey. > >>>>>>>> > >>>>>>>> I just repeat our agreement to be on the same page > >>>>>>>> > >>>>>>>>> The confirmation should only present in the user-facing > interfaces. > >>>>>>>> 1. We should add —force flag to the command.sh deactivation > command. > >>>>>>>> 2. We should throw the exception if cluster has in-memory caches > and > >>>>>>>> —force=false. > >>>>>>>> 3. We shouldn’t change Java API for deactivation. > >>>>>>>> > >>>>>>>> Is it correct? > >>>>>>>> > >>>>>>>>> The DROP TABLE command does not have a "yes I am sure" clause in > it > >>>>>>>> I think it because the command itself has a «DROP» word in it’s > >> name. > >>>>>>>> Which clearly explains what will happen on it’s execution. > >>>>>>>> > >>>>>>>> On the opposite «deactivation» command doesn’t have any sign of > >>>>>>>> destructive operation. > >>>>>>>> That’s why we should warn user about it’s consequences. > >>>>>>>> > >>>>>>>> > >>>>>>>>> 24 марта 2020 г., в 12:38, Alexey Goncharuk < > >>>>>> alexey.goncha...@gmail.com> > >>>>>>>> написал(а): > >>>>>>>>> Igniters, Ivan, Nikolay, > >>>>>>>>> > >>>>>>>>> I am strongly against adding confirmation flags to any kind of > >> APIs, > >>>>>>>>> whether we change the deactivation behavior or not (even though I > >>>> agree > >>>>>>>>> that it makes sense to fix the deactivation to not clean up the > >>>>>> in-memory > >>>>>>>>> data). The confirmation should only present in the user-facing > >>>>>>>> interfaces. > >>>>>>>>> I cannot recall any software interface which has such a flag. > None > >> of > >>>>>> the > >>>>>>>>> syscalls which delete files (a very destructive operation) have > >> this > >>>>>>>> flag. > >>>>>>>>> The DROP TABLE command does not have a "yes I am sure" clause in > >> it. > >>>>>> As I > >>>>>>>>> already mentioned, when used programmatically, most users will > >> likely > >>>>>>>>> simply pass 'true' as the new flag because they already know the > >>>>>>>> behavior. > >>>>>>>>> This is a clear sign of a bad design choice. > >>>>>>>>> > >>>>>>>>> On top of that, given that it is our intention to change the > >> behavior > >>>>>> of > >>>>>>>>> deactivation to not loose the in-memory data, it does not make > >> sense > >>>> to > >>>>>>>> me > >>>>>>>>> to change the API. >