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.
>
