Hi Vladimir,
> There are about 15 places in inner logic with this description.
> I propose balance between code base size and comment completeness.
I agree with Iva and I also think that this approach is not so good.
Perhaps we can add just a link to the one method which will provide a
comprehensive description, something like as follows
@param forceDeactivation {@code true} if cluster deactivation should be
forced. Please take a look at {@link IgniteCluster#state(ClusterState
newState, boolean force)} for the details.
What do you think?
Thanks,
Slava.
чт, 2 апр. 2020 г. в 18:47, Vladimir Steshin <vlads...@gmail.com>:
> Ivan, hello.
>
> Thanks. I vote for keeping the comments as they are now :)
>
> Igniters, it seems we are agreed to merge [1]. And the ticked s to be
> reverted in future with new designed solution of keeping in-memory data
> after deactivation.
>
> Right?
>
>
> [1] https://issues.apache.org/jira/browse/IGNITE-12779
>
>
> 01.04.2020 20:20, Ivan Rakov пишет:
> > 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.
>
