Re: Issue with full disk store directories
Hi, An update about this issue. I think the problem is in the PersistentOplogSet class, in the following method: /** * Returns the next available DirectoryHolder which has space. If no dir has space then it will * return one anyway if compaction is enabled. * * @param minAvailableSpace the minimum amount of space we need in this directory. */ DirectoryHolder getNextDir(int minAvailableSpace, boolean checkForWarning) In order to select a directory, this check is performed: if (dirHolder.getAvailableSpace() >= minAvailableSpace) But I think it should compare the available space with the size of the oplog files, as they are created with the maximun size. After changing the check by: if (dirHolder.getAvailableSpace() >= parent.getMaxOplogSizeInBytes()) then the full folder is skipped, and the next one is selected. I'll try to write a test to ilustrate this. BR/ Alberto B. De: Alberto Bustamante Reyes Enviado: lunes, 6 de mayo de 2019 10:04:34 Para: dev@geode.apache.org Asunto: RE: Issue with full disk store directories Not sure if I understand your question: in the test I did, I used one disk store composed by three directories, each one with different size. These directories were in the same disk partition. The issue I saw is that when the log files are initialized, it is not checked if they fit in the directory, so if the maximum directory sized is reached, the server crashes. De: Anthony Baker Enviado: lunes, 29 de abril de 2019 17:24 Para: dev@geode.apache.org Asunto: Re: Issue with full disk store directories Question: are you using similarly sized disk partitioned for all your disk stores? > On Apr 24, 2019, at 3:42 AM, Alberto Bustamante Reyes > wrote: > > Hi all, > > I reported an issue in Jira, related with full disk store directories: > https://issues.apache.org/jira/browse/GEODE-6652 > As I describe there, the issue is that when using a disk store with > directories of different sizes, when oplog files rotate, the available space > of the next disk store directory to be used seems not to be checked correctly. > > BR/ > > Alberto > >
Re: Issue with full disk store directories
Thanks for investigating this. Looking forward to hearing the results of your testing. Note that when we create an oplog file we allocate all the bytes (including writing to each disk page) to avoid corruption if the disk becomes full while writing records. Anthony > On Jun 7, 2019, at 4:26 AM, Alberto Bustamante Reyes > wrote: > > Hi, > > > An update about this issue. I think the problem is in the PersistentOplogSet > class, in the following method: > > > /** > * Returns the next available DirectoryHolder which has space. If no dir has > space then it will > * return one anyway if compaction is enabled. > * > * @param minAvailableSpace the minimum amount of space we need in this > directory. > */ > DirectoryHolder getNextDir(int minAvailableSpace, boolean checkForWarning) > > > > > In order to select a directory, this check is performed: > > > if (dirHolder.getAvailableSpace() >= minAvailableSpace) > > > But I think it should compare the available space with the size of the oplog > files, as they are created with the maximun size. > > After changing the check by: > > > if (dirHolder.getAvailableSpace() >= parent.getMaxOplogSizeInBytes()) > > > then the full folder is skipped, and the next one is selected. > > > > I'll try to write a test to ilustrate this. > > > BR/ > > > Alberto B. > > > > > De: Alberto Bustamante Reyes > Enviado: lunes, 6 de mayo de 2019 10:04:34 > Para: dev@geode.apache.org > Asunto: RE: Issue with full disk store directories > > Not sure if I understand your question: in the test I did, I used one disk > store composed by three directories, each one with different size. These > directories were in the same disk partition. The issue I saw is that when the > log files are initialized, it is not checked if they fit in the directory, so > if the maximum directory sized is reached, the server crashes. > > De: Anthony Baker > Enviado: lunes, 29 de abril de 2019 17:24 > Para: dev@geode.apache.org > Asunto: Re: Issue with full disk store directories > > Question: are you using similarly sized disk partitioned for all your disk > stores? > >> On Apr 24, 2019, at 3:42 AM, Alberto Bustamante Reyes >> wrote: >> >> Hi all, >> >> I reported an issue in Jira, related with full disk store directories: >> https://issues.apache.org/jira/browse/GEODE-6652 >> As I describe there, the issue is that when using a disk store with >> directories of different sizes, when oplog files rotate, the available space >> of the next disk store directory to be used seems not to be checked >> correctly. >> >> BR/ >> >> Alberto >> >> >
[DISCUSS] changing geode 32-bit counter stats to 64-bit
We have had a couple of tickets that have problems with 32-bit counters changing too fast and causing them to be hard to understand when they wrap around (see GEODE-6425 and GEODE-6834). We have also had problems with some stats being broken because they were changing the 32-bit one when they should have been changing the 64-bit one (see GEODE-6776). The current implementation has one array of values for the 32-bit stats and another array of values for the 64-bit stats. We use indexes into these arrays when changing a stat. Given an int "id" used to index these arrays, we can not tell if we should be indexing the 32-bit array or 64-bit array. The first 32-bit stat for a type will have an id of 0 and the first 64-bit stat on that type will also have an id of 0. But our current implementation has the same type of value in both arrays (LongAdder see: StripedStatisticsImpl fields intAdders and longAdders). So if we simply change our implementation to have a single array, then the ids will no longer be overloaded. Changing this internal implementation also improves backward compatibility. Currently when we change one of our counters from 32-bit to 64-bit it is possible we would break existing applications that are using the Statistics interface. It has methods on it that allow stats to be read given an it: getInt(int id) and getLong(int id). If they are currently reading a 32-bit stat using getInt(id) and we change that stat to be 64-bit (like we did in GEODE-6425) then they will now be reading the wrong stat when they call getInt(id). If they used getInt(String) or getInt(StatisticDescriptor) they will be okay. But if we make this simple change to have 32-bit and 64-bit stats stored in the same array then getInt(id) will do the right thing when we change a stat from 32-bit to 64-bit. Does anyone see a problem with making this change? After we do it I think we should change all of our counters to 64-bit since they are always stored in a LongAdder anyway and we should deprecate the methods that support 32-bit stats.
Re: [DISCUSS] changing geode 32-bit counter stats to 64-bit
I like this! I’d go ahead and change all the usage of the int methods to the long methods. I’d deprecate the int methods to make it very clear. If some consumer is using the int methods they will still work with the same rollover issues but perhaps with the deprecated warning they will update their code. Without the warning they may never know. -jake > On Jun 7, 2019, at 1:32 PM, Darrel Schneider wrote: > > We have had a couple of tickets that have problems with 32-bit counters > changing too fast and causing them to be hard to understand when they wrap > around (see GEODE-6425 and GEODE-6834). We have also had problems with some > stats being broken because they were changing the 32-bit one when they > should have been changing the 64-bit one (see GEODE-6776). > The current implementation has one array of values for the 32-bit stats and > another array of values for the 64-bit stats. We use indexes into these > arrays when changing a stat. Given an int "id" used to index these arrays, > we can not tell if we should be indexing the 32-bit array or 64-bit array. > The first 32-bit stat for a type will have an id of 0 and the first 64-bit > stat on that type will also have an id of 0. But our current implementation > has the same type of value in both arrays (LongAdder > see: StripedStatisticsImpl fields intAdders and longAdders). So if we > simply change our implementation to have a single array, then the ids will > no longer be overloaded. > > Changing this internal implementation also improves backward compatibility. > Currently when we change one of our counters from 32-bit to 64-bit it is > possible we would break existing applications that are using the Statistics > interface. It has methods on it that allow stats to be read given an it: > getInt(int id) and getLong(int id). If they are currently reading a 32-bit > stat using getInt(id) and we change that stat to be 64-bit (like we did in > GEODE-6425) then they will now be reading the wrong stat when they call > getInt(id). If they used getInt(String) or getInt(StatisticDescriptor) they > will be okay. But if we make this simple change to have 32-bit and 64-bit > stats stored in the same array then getInt(id) will do the right thing when > we change a stat from 32-bit to 64-bit. > > Does anyone see a problem with making this change? > > After we do it I think we should change all of our counters to 64-bit since > they are always stored in a LongAdder anyway and we should deprecate the > methods that support 32-bit stats.
Re: [DISCUSS] changing geode 32-bit counter stats to 64-bit
+1 - this sounds like a good change to me. -Dan On Fri, Jun 7, 2019 at 1:47 PM Jacob Barrett wrote: > I like this! > > I’d go ahead and change all the usage of the int methods to the long > methods. I’d deprecate the int methods to make it very clear. > > If some consumer is using the int methods they will still work with the > same rollover issues but perhaps with the deprecated warning they will > update their code. Without the warning they may never know. > > -jake > > > > On Jun 7, 2019, at 1:32 PM, Darrel Schneider > wrote: > > > > We have had a couple of tickets that have problems with 32-bit counters > > changing too fast and causing them to be hard to understand when they > wrap > > around (see GEODE-6425 and GEODE-6834). We have also had problems with > some > > stats being broken because they were changing the 32-bit one when they > > should have been changing the 64-bit one (see GEODE-6776). > > The current implementation has one array of values for the 32-bit stats > and > > another array of values for the 64-bit stats. We use indexes into these > > arrays when changing a stat. Given an int "id" used to index these > arrays, > > we can not tell if we should be indexing the 32-bit array or 64-bit > array. > > The first 32-bit stat for a type will have an id of 0 and the first > 64-bit > > stat on that type will also have an id of 0. But our current > implementation > > has the same type of value in both arrays (LongAdder > > see: StripedStatisticsImpl fields intAdders and longAdders). So if we > > simply change our implementation to have a single array, then the ids > will > > no longer be overloaded. > > > > Changing this internal implementation also improves backward > compatibility. > > Currently when we change one of our counters from 32-bit to 64-bit it is > > possible we would break existing applications that are using the > Statistics > > interface. It has methods on it that allow stats to be read given an it: > > getInt(int id) and getLong(int id). If they are currently reading a > 32-bit > > stat using getInt(id) and we change that stat to be 64-bit (like we did > in > > GEODE-6425) then they will now be reading the wrong stat when they call > > getInt(id). If they used getInt(String) or getInt(StatisticDescriptor) > they > > will be okay. But if we make this simple change to have 32-bit and 64-bit > > stats stored in the same array then getInt(id) will do the right thing > when > > we change a stat from 32-bit to 64-bit. > > > > Does anyone see a problem with making this change? > > > > After we do it I think we should change all of our counters to 64-bit > since > > they are always stored in a LongAdder anyway and we should deprecate the > > methods that support 32-bit stats. >
Re: [DISCUSS] changing geode 32-bit counter stats to 64-bit
+1, if the LongAdders have already added and the additional memory usage has already been dealt with, then adding the accessors does not really make a difference anymore. On 6/7/19 13:47, Jacob Barrett wrote: I like this! I’d go ahead and change all the usage of the int methods to the long methods. I’d deprecate the int methods to make it very clear. If some consumer is using the int methods they will still work with the same rollover issues but perhaps with the deprecated warning they will update their code. Without the warning they may never know. -jake On Jun 7, 2019, at 1:32 PM, Darrel Schneider wrote: We have had a couple of tickets that have problems with 32-bit counters changing too fast and causing them to be hard to understand when they wrap around (see GEODE-6425 and GEODE-6834). We have also had problems with some stats being broken because they were changing the 32-bit one when they should have been changing the 64-bit one (see GEODE-6776). The current implementation has one array of values for the 32-bit stats and another array of values for the 64-bit stats. We use indexes into these arrays when changing a stat. Given an int "id" used to index these arrays, we can not tell if we should be indexing the 32-bit array or 64-bit array. The first 32-bit stat for a type will have an id of 0 and the first 64-bit stat on that type will also have an id of 0. But our current implementation has the same type of value in both arrays (LongAdder see: StripedStatisticsImpl fields intAdders and longAdders). So if we simply change our implementation to have a single array, then the ids will no longer be overloaded. Changing this internal implementation also improves backward compatibility. Currently when we change one of our counters from 32-bit to 64-bit it is possible we would break existing applications that are using the Statistics interface. It has methods on it that allow stats to be read given an it: getInt(int id) and getLong(int id). If they are currently reading a 32-bit stat using getInt(id) and we change that stat to be 64-bit (like we did in GEODE-6425) then they will now be reading the wrong stat when they call getInt(id). If they used getInt(String) or getInt(StatisticDescriptor) they will be okay. But if we make this simple change to have 32-bit and 64-bit stats stored in the same array then getInt(id) will do the right thing when we change a stat from 32-bit to 64-bit. Does anyone see a problem with making this change? After we do it I think we should change all of our counters to 64-bit since they are always stored in a LongAdder anyway and we should deprecate the methods that support 32-bit stats.
