Ivan, Instead of “setStoragePath” I would suggest “setPersistencePath”. Left some extra notes in the ticket.
— Denis > On Oct 11, 2017, at 4:30 AM, Ivan Rakov <ivan.glu...@gmail.com> wrote: > > Vladimir, > > Thanks for focusing on existing namings. Most of your suggestions really > sound better. I've posted my thoughts under your comment. > > By the way, we should decide two things: > > 1) Naming for methods for configuring store path. I suggest the following: > > *setStoragePath* - for partition and index files > *setWalPath* - for WAL files > *walArchivePath* - for WAL archive files > > 2) Renaming *checkpointingFrequency* to *checkpointFrequency* (same with > *checkpointingPageBufferSize* and *checkpointingThreads*). Both options > sounds ok to me, let's see what community thinks. > > Best Regards, > Ivan Rakov > > On 11.10.2017 14:05, Vladimir Ozerov wrote: >> Ivan, >> >> I left some comments in the ticket [1], please take a look. >> >> [1] >> https://issues.apache.org/jira/browse/IGNITE-6030?focusedCommentId=16200050&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16200050 >> >> On Wed, Oct 11, 2017 at 12:04 PM, Ivan Rakov <ivan.glu...@gmail.com> wrote: >> >>> Igniters, >>> >>> https://issues.apache.org/jira/browse/IGNITE-6030 is ready and enqueued >>> for TC run. >>> PR: https://github.com/apache/ignite/pull/2828 >>> >>> Everyone interested in new data storage configuration API, please pay >>> attention and review. >>> >>> >>> Best Regards, >>> Ivan Rakov >>> >>> >>> On 09.10.2017 12:40, Pavel Tupitsyn wrote: >>> >>>> Sounds good to me. >>>> >>>> On Mon, Oct 9, 2017 at 12:35 PM, Ivan Rakov <ivan.glu...@gmail.com> >>>> wrote: >>>> >>>> Pavel, >>>>> Sounds reasonable. >>>>> I suggest to include both "data" and "configuration" to make it even more >>>>> obvious: >>>>> >>>>> set/getDefaultDataRegionConfiguration >>>>> set/getDataRegionConfigurations >>>>> >>>>> Best Regards, >>>>> Ivan Rakov >>>>> >>>>> >>>>> On 09.10.2017 10:51, Pavel Tupitsyn wrote: >>>>> >>>>> Sorry that I'm late to the party, but this looks inconsistent: >>>>>> DataStorageConfiguration defaultRegionConfiguration >>>>>> DataRegionConfiguration[] getDataRegions >>>>>> >>>>>> defaultRegionConfiguration + getRegionConfigurations >>>>>> - or - >>>>>> defaultDataRegion + getDataRegions >>>>>> >>>>>> Thoughts? >>>>>> >>>>>> On Mon, Oct 2, 2017 at 9:10 PM, Ivan Rakov <ivan.glu...@gmail.com> >>>>>> wrote: >>>>>> >>>>>> Denis, >>>>>> >>>>>>> Yes, you're right. All cache groups without specific data region >>>>>>> configured will be persistent. >>>>>>> And if you want to add another persistent data region, you should set >>>>>>> *isPeristenceEnabled* flag in its *DataRegionConfiguration* explictly. >>>>>>> >>>>>>> Best Regards, >>>>>>> Ivan Rakov >>>>>>> >>>>>>> >>>>>>> On 02.10.2017 21:01, Denis Magda wrote: >>>>>>> >>>>>>> Missed the point with defaults. Makes sense to me now. So to wrap this >>>>>>> >>>>>>>> up, if I want to enable the persistence globally and don’t have any >>>>>>>> regions >>>>>>>> configured explicitly I need to take the default region and switch the >>>>>>>> persistence on for it. Is my understanding correct? >>>>>>>> >>>>>>>> — >>>>>>>> Denis >>>>>>>> >>>>>>>> On Oct 2, 2017, at 10:57 AM, Ivan Rakov <ivan.glu...@gmail.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>> Denis, why do you need to access an instance of the default region >>>>>>>>> bean? >>>>>>>>> If you want to set any parameter, just instantiate new bean with this >>>>>>>>> parameter set (like in XML snipped below). Other parameters will be >>>>>>>>> automatically initialized with their default values. >>>>>>>>> >>>>>>>>> Best Regards, >>>>>>>>> Ivan Rakov >>>>>>>>> >>>>>>>>> On 02.10.2017 19:28, Denis Magda wrote: >>>>>>>>> >>>>>>>>> <property name="dataStorageConfiguration"> >>>>>>>>> >>>>>>>>>> <bean class="org.apache.ignite.confi >>>>>>>>>>>> guration.DataStorageConfiguration"> >>>>>>>>>>>> <property name="systemCacheInitialSize" >>>>>>>>>>>> value="#{100 >>>>>>>>>>>> * >>>>>>>>>>>> 1024 * 1024}"/> >>>>>>>>>>>> <property name="defaultRegionConfiguration"> >>>>>>>>>>>> <bean class="org.apache.ignite.confi >>>>>>>>>>>> guration.DataRegionConfiguration"> >>>>>>>>>>>> <property name="maxSize" value="#{5 * >>>>>>>>>>>> 1024 * >>>>>>>>>>>> 102 * 1024}"/> >>>>>>>>>>>> </bean> >>>>>>>>>>>> </property> >>>>>>>>>>>> </bean> >>>>>>>>>>>> </property> >>>>>>>>>>>> >>>>>>>>>>>> In other data regions persistence will be disabled by default. >>>>>>>>>>>> >>>>>>>>>>> Ivan, how to get an instance to the default region bean and change >>>>>>>>>>> a >>>>>>>>>>> >>>>>>>>>> parameter? Obviously, if the goal is to enable the persistence I >>>>>>>>>> don’t want >>>>>>>>>> to create the default region bean from scratch. >>>>>>>>>> >>>>>>>>>> — >>>>>>>>>> Denis >>>>>>>>>> >>>>>>>>>> On Oct 2, 2017, at 9:11 AM, Ivan Rakov <ivan.glu...@gmail.com> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Agree with Alexey. >>>>>>>>>>> Properties like *defaultDataRegionSize*, >>>>>>>>>>> *isDefaultPersistenceEnabled* >>>>>>>>>>> can confuse users who don't know that there's such thing as default >>>>>>>>>>> data >>>>>>>>>>> region. They can decide they are inherited by all data regions >>>>>>>>>>> where >>>>>>>>>>> size >>>>>>>>>>> and persistence flag are not explicitly set. >>>>>>>>>>> >>>>>>>>>>> Let's get rid of these properties and add >>>>>>>>>>> *defaultRegionConfiguration* >>>>>>>>>>> property with explicit configuration of default data region. >>>>>>>>>>> >>>>>>>>>>> Regarding XML configuration, changing size or persistence flag of >>>>>>>>>>> default data region will be just two lines longer (for bean >>>>>>>>>>> description): >>>>>>>>>>> >>>>>>>>>>> <property name="dataStorageConfiguration"> >>>>>>>>>>> >>>>>>>>>>> <bean class="org.apache.ignite.confi >>>>>>>>>>>> guration.DataStorageConfiguration"> >>>>>>>>>>>> <property name="systemCacheInitialSize" >>>>>>>>>>>> value="#{100 >>>>>>>>>>>> * >>>>>>>>>>>> 1024 * 1024}"/> >>>>>>>>>>>> <property name="defaultRegionConfiguration"> >>>>>>>>>>>> <bean class="org.apache.ignite.confi >>>>>>>>>>>> guration.DataRegionConfiguration"> >>>>>>>>>>>> <property name="maxSize" value="#{5 * >>>>>>>>>>>> 1024 * >>>>>>>>>>>> 102 * 1024}"/> >>>>>>>>>>>> </bean> >>>>>>>>>>>> </property> >>>>>>>>>>>> </bean> >>>>>>>>>>>> </property> >>>>>>>>>>>> >>>>>>>>>>>> In other data regions persistence will be disabled by default. >>>>>>>>>>>> >>>>>>>>>>> I've updated draft in https://issues.apache.org/jira >>>>>>>>>>> /browse/IGNITE-6030 with these changes. >>>>>>>>>>> >>>>>>>>>>> Best Regards, >>>>>>>>>>> Ivan Rakov >>>>>>>>>>> >>>>>>>>>>> On 02.10.2017 18:35, Denis Magda wrote: >>>>>>>>>>> >>>>>>>>>>> To resolve this, I suggest to >>>>>>>>>>> >>>>>>>>>>>> introduce just another field defaultRegionConfiguration and get >>>>>>>>>>>>> rid >>>>>>>>>>>>> of >>>>>>>>>>>>> other defaults in DataStorageConfiguration. >>>>>>>>>>>>> >>>>>>>>>>>>> Won’t it complicate the configuration from a Spring XML file? I’m >>>>>>>>>>>>> >>>>>>>>>>>> not >>>>>>>>>>>> an expert in Spring so how do I get defaultRegionConfiguration >>>>>>>>>>>> bean >>>>>>>>>>>> first >>>>>>>>>>>> to change any parameter? >>>>>>>>>>>> >>>>>>>>>>>> — >>>>>>>>>>>> Denis >>>>>>>>>>>> >>>>>>>>>>>> On Oct 2, 2017, at 8:30 AM, Alexey Goncharuk < >>>>>>>>>>>> >>>>>>>>>>>> alexey.goncha...@gmail.com> wrote: >>>>>>>>>>>>> Agree with Vladimir. If we are to implement this, we would either >>>>>>>>>>>>> need to >>>>>>>>>>>>> have a Boolean (non-primitive) for persistenceEnabled on >>>>>>>>>>>>> DataRegionConfiguration, or introduce an enum for this field >>>>>>>>>>>>> which >>>>>>>>>>>>> is also >>>>>>>>>>>>> an overkill. On the other hand, one can assume that the defaults >>>>>>>>>>>>> we >>>>>>>>>>>>> are >>>>>>>>>>>>> talking about are actually inherited. To resolve this, I suggest >>>>>>>>>>>>> to >>>>>>>>>>>>> introduce just another field defaultRegionConfiguration and get >>>>>>>>>>>>> rid >>>>>>>>>>>>> of >>>>>>>>>>>>> other defaults in DataStorageConfiguration. >>>>>>>>>>>>> >>>>>>>>>>>>> Thoughts? >>>>>>>>>>>>> >>>>>>>>>>>>> 2017-10-02 15:19 GMT+03:00 Ivan Rakov <ivan.glu...@gmail.com>: >>>>>>>>>>>>> >>>>>>>>>>>>> Vladimir, >>>>>>>>>>>>> >>>>>>>>>>>>> I like your approach because it's easier to implement. >>>>>>>>>>>>>> However, user may be confused by setting >>>>>>>>>>>>>> *isDefaultPersistenceEnabled* >>>>>>>>>>>>>> flag and seeing that persistence is not enabled by default in >>>>>>>>>>>>>> custom memory >>>>>>>>>>>>>> region. I'll add clarifying Javadoc at this place. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Best Regards, >>>>>>>>>>>>>> Ivan Rakov >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 02.10.2017 11:28, Vladimir Ozerov wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Ivan, >>>>>>>>>>>>>> >>>>>>>>>>>>>> I do not think this is correct approach, because it will be hard >>>>>>>>>>>>>>> to >>>>>>>>>>>>>>> explain, and you will have to use "Boolean" instead of >>>>>>>>>>>>>>> "boolean" >>>>>>>>>>>>>>> for >>>>>>>>>>>>>>> DataRegionConfiguration. I do not think we need default >>>>>>>>>>>>>>> "persistence >>>>>>>>>>>>>>> enabled" for all regions. Instead, we should have "persistence >>>>>>>>>>>>>>> enabled" >>>>>>>>>>>>>>> flag for default region only. It should not be propagated to >>>>>>>>>>>>>>> custom >>>>>>>>>>>>>>> regions. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Mon, Oct 2, 2017 at 11:14 AM, Ivan Rakov < >>>>>>>>>>>>>>> ivan.glu...@gmail.com >>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Guys, I think I got the point now. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Let's check the final design: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> *DataStorageConfiguration* will have >>>>>>>>>>>>>>>> *isDefaultPersistenceEnabled* >>>>>>>>>>>>>>>> property (default = false), which will be used for enabling >>>>>>>>>>>>>>>> persistence >>>>>>>>>>>>>>>> in >>>>>>>>>>>>>>>> default data region. >>>>>>>>>>>>>>>> *DataRegionConfiguration* will have *isPersistenceEnabled* >>>>>>>>>>>>>>>> property, >>>>>>>>>>>>>>>> which >>>>>>>>>>>>>>>> will be used for enabling persistence in corresponding data >>>>>>>>>>>>>>>> region. If >>>>>>>>>>>>>>>> value is not set, value of *DataStorageConfiguration::isD >>>>>>>>>>>>>>>> efaultPersistenceEnabled* >>>>>>>>>>>>>>>> will be used by default. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Best Regards, >>>>>>>>>>>>>>>> Ivan Rakov >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On 02.10.2017 7:49, Dmitriy Setrakyan wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Mon, Oct 2, 2017 at 7:46 AM, Denis Magda < >>>>>>>>>>>>>>>> dma...@apache.org> >>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Oct 1, 2017, at 4:41 AM, Ivan Rakov <ivan.glu...@gmail.com >>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> 1) You're right. I forgot to include the main flag in >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> DataRegionConfiguration - *isPersistenceEnabled*. Persistence >>>>>>>>>>>>>>>>>>> will be >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> enabled globally if at least one memory region has this >>>>>>>>>>>>>>>>>>> flag >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> set. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> I’m confused. Why the persistence should be enabled >>>>>>>>>>>>>>>>>> *globally* >>>>>>>>>>>>>>>>>> if the >>>>>>>>>>>>>>>>>> purpose is to have it set for a specific region? If it’s >>>>>>>>>>>>>>>>>> enabled for >>>>>>>>>>>>>>>>>> region >>>>>>>>>>>>>>>>>> A only, I don’t want to have it activated for region B. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Yes, you are right. By default the persistence will be >>>>>>>>>>>>>>>>>> disabled >>>>>>>>>>>>>>>>>> globally. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> But we should also give users a way to switch the default >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> behavior from >>>>>>>>>>>>>>>>> in-memory only (no-persistence) to persistence. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >
