Re: Old geode-benchmark PRs
Why not just leave the PR open then? > On Jan 23, 2020, at 9:52 AM, Robert Houghton wrote: > > @Alberto I like this idea. The link between Jira and the GitHub PR will be > a nice breadcrumb to follow going forward as well, assuming the PRs are > correctly labeled for cross-linking to occur. > > > On Thu, Jan 23, 2020 at 8:05 AM Alberto Bustamante Reyes > wrote: > >> What about closing the PRs and creating a Jira ticket (or tickets) for the >> review and update of the code? >> If someone finds time to spend on benchmarks, at least he/she will find >> the tickets in Jira. >> >> >> >> De: Donal Evans >> Enviado: jueves, 23 de enero de 2020 16:45 >> Para: dev@geode.apache.org >> Asunto: Re: Old geode-benchmark PRs >> >> @Alexander, I haven't looked at them in months and they never received any >> formal review on GitHub, so it's hard to know for sure if they're ready to >> merge or not, but as Jake said, they probably need some massaging to get >> the resource usage just right and minimize variance. If at this point >> there's no-one who knows enough about tuning benchmarks with the time to >> look at them, then it seems unlikely that they'll get merged any time soon. >> >> On Thu, Jan 23, 2020 at 6:42 AM Alexander Murmann >> wrote: >> >>> Donal, are you still looking at these? If they aren't ready to merge and >>> not being worked on, should they be closed? >>> >>> On Wed, Jan 22, 2020 at 3:32 PM Donal Evans wrote: >>> >>>> Two of those PRs are mine, so perhaps I can give a bit of context for >>>> people who might look at them. The oldest of the two, "Feature/Add >>> PdxType >>>> benchmark and additional framework flexibility" was an attempt to >>> quantify >>>> and maintain the improvement in performance for PdxType creation when >>> large >>>> numbers of PdxTypes already exist, and to allow the passing of >> additional >>>> system properties to the VMs hosting the servers in order to change the >>> log >>>> level and prevent the benchmark measuring how long it takes to log >>> PdxType >>>> creation rather than actual time taken to create new PdxTypes. This PR >>> has >>>> been open for a very long time, so it's possible that the changes >>> regarding >>>> passing additional system properties to the VMs are now outdated or >>>> unnecessary, but the actual benchmarks themselves still have some >> value. >>>> >>>> The second PR, "Added benchmarks for aggregate functions" contains 16 >> new >>>> benchmarks related to aggregate OQL queries, (8 each for Partitioned >> and >>>> Replicated regions), which were added following work in that area by >> the >>>> Commons team. The build is currently marked as failing, but this is due >>> to >>>> a timeout rather than an actual build failure, as the number of >>> benchmarks >>>> added increased the total time to build beyond the currently configured >>>> timeout. Adding such a large number of additional benchmarks will >>> probably >>>> also noticeably increase the time it takes benchmarks to run, which >> bears >>>> consideration. >>>> >>>> I hope this helps shed some light for people who may look over those >> PRs. >>>> >>>> On Wed, Jan 22, 2020 at 11:36 AM Dan Smith wrote: >>>> >>>>> Hi, >>>>> >>>>> I noticed we have some old outstanding PRs for the geode-benchmarks >>>>> project. Are any of these things we want to merge or should we close >>> them >>>>> out? >>>>> >>>>> https://github.com/apache/geode-benchmarks/pulls >>>>> >>>>> -Dan >>>>> >>>> >>> >>
Re: Old geode-benchmark PRs
@Alberto I like this idea. The link between Jira and the GitHub PR will be a nice breadcrumb to follow going forward as well, assuming the PRs are correctly labeled for cross-linking to occur. On Thu, Jan 23, 2020 at 8:05 AM Alberto Bustamante Reyes wrote: > What about closing the PRs and creating a Jira ticket (or tickets) for the > review and update of the code? > If someone finds time to spend on benchmarks, at least he/she will find > the tickets in Jira. > > > > De: Donal Evans > Enviado: jueves, 23 de enero de 2020 16:45 > Para: dev@geode.apache.org > Asunto: Re: Old geode-benchmark PRs > > @Alexander, I haven't looked at them in months and they never received any > formal review on GitHub, so it's hard to know for sure if they're ready to > merge or not, but as Jake said, they probably need some massaging to get > the resource usage just right and minimize variance. If at this point > there's no-one who knows enough about tuning benchmarks with the time to > look at them, then it seems unlikely that they'll get merged any time soon. > > On Thu, Jan 23, 2020 at 6:42 AM Alexander Murmann > wrote: > > > Donal, are you still looking at these? If they aren't ready to merge and > > not being worked on, should they be closed? > > > > On Wed, Jan 22, 2020 at 3:32 PM Donal Evans wrote: > > > > > Two of those PRs are mine, so perhaps I can give a bit of context for > > > people who might look at them. The oldest of the two, "Feature/Add > > PdxType > > > benchmark and additional framework flexibility" was an attempt to > > quantify > > > and maintain the improvement in performance for PdxType creation when > > large > > > numbers of PdxTypes already exist, and to allow the passing of > additional > > > system properties to the VMs hosting the servers in order to change the > > log > > > level and prevent the benchmark measuring how long it takes to log > > PdxType > > > creation rather than actual time taken to create new PdxTypes. This PR > > has > > > been open for a very long time, so it's possible that the changes > > regarding > > > passing additional system properties to the VMs are now outdated or > > > unnecessary, but the actual benchmarks themselves still have some > value. > > > > > > The second PR, "Added benchmarks for aggregate functions" contains 16 > new > > > benchmarks related to aggregate OQL queries, (8 each for Partitioned > and > > > Replicated regions), which were added following work in that area by > the > > > Commons team. The build is currently marked as failing, but this is due > > to > > > a timeout rather than an actual build failure, as the number of > > benchmarks > > > added increased the total time to build beyond the currently configured > > > timeout. Adding such a large number of additional benchmarks will > > probably > > > also noticeably increase the time it takes benchmarks to run, which > bears > > > consideration. > > > > > > I hope this helps shed some light for people who may look over those > PRs. > > > > > > On Wed, Jan 22, 2020 at 11:36 AM Dan Smith wrote: > > > > > > > Hi, > > > > > > > > I noticed we have some old outstanding PRs for the geode-benchmarks > > > > project. Are any of these things we want to merge or should we close > > them > > > > out? > > > > > > > > https://github.com/apache/geode-benchmarks/pulls > > > > > > > > -Dan > > > > > > > > > >
RE: Old geode-benchmark PRs
What about closing the PRs and creating a Jira ticket (or tickets) for the review and update of the code? If someone finds time to spend on benchmarks, at least he/she will find the tickets in Jira. De: Donal Evans Enviado: jueves, 23 de enero de 2020 16:45 Para: dev@geode.apache.org Asunto: Re: Old geode-benchmark PRs @Alexander, I haven't looked at them in months and they never received any formal review on GitHub, so it's hard to know for sure if they're ready to merge or not, but as Jake said, they probably need some massaging to get the resource usage just right and minimize variance. If at this point there's no-one who knows enough about tuning benchmarks with the time to look at them, then it seems unlikely that they'll get merged any time soon. On Thu, Jan 23, 2020 at 6:42 AM Alexander Murmann wrote: > Donal, are you still looking at these? If they aren't ready to merge and > not being worked on, should they be closed? > > On Wed, Jan 22, 2020 at 3:32 PM Donal Evans wrote: > > > Two of those PRs are mine, so perhaps I can give a bit of context for > > people who might look at them. The oldest of the two, "Feature/Add > PdxType > > benchmark and additional framework flexibility" was an attempt to > quantify > > and maintain the improvement in performance for PdxType creation when > large > > numbers of PdxTypes already exist, and to allow the passing of additional > > system properties to the VMs hosting the servers in order to change the > log > > level and prevent the benchmark measuring how long it takes to log > PdxType > > creation rather than actual time taken to create new PdxTypes. This PR > has > > been open for a very long time, so it's possible that the changes > regarding > > passing additional system properties to the VMs are now outdated or > > unnecessary, but the actual benchmarks themselves still have some value. > > > > The second PR, "Added benchmarks for aggregate functions" contains 16 new > > benchmarks related to aggregate OQL queries, (8 each for Partitioned and > > Replicated regions), which were added following work in that area by the > > Commons team. The build is currently marked as failing, but this is due > to > > a timeout rather than an actual build failure, as the number of > benchmarks > > added increased the total time to build beyond the currently configured > > timeout. Adding such a large number of additional benchmarks will > probably > > also noticeably increase the time it takes benchmarks to run, which bears > > consideration. > > > > I hope this helps shed some light for people who may look over those PRs. > > > > On Wed, Jan 22, 2020 at 11:36 AM Dan Smith wrote: > > > > > Hi, > > > > > > I noticed we have some old outstanding PRs for the geode-benchmarks > > > project. Are any of these things we want to merge or should we close > them > > > out? > > > > > > https://github.com/apache/geode-benchmarks/pulls > > > > > > -Dan > > > > > >
Re: Old geode-benchmark PRs
@Alexander, I haven't looked at them in months and they never received any formal review on GitHub, so it's hard to know for sure if they're ready to merge or not, but as Jake said, they probably need some massaging to get the resource usage just right and minimize variance. If at this point there's no-one who knows enough about tuning benchmarks with the time to look at them, then it seems unlikely that they'll get merged any time soon. On Thu, Jan 23, 2020 at 6:42 AM Alexander Murmann wrote: > Donal, are you still looking at these? If they aren't ready to merge and > not being worked on, should they be closed? > > On Wed, Jan 22, 2020 at 3:32 PM Donal Evans wrote: > > > Two of those PRs are mine, so perhaps I can give a bit of context for > > people who might look at them. The oldest of the two, "Feature/Add > PdxType > > benchmark and additional framework flexibility" was an attempt to > quantify > > and maintain the improvement in performance for PdxType creation when > large > > numbers of PdxTypes already exist, and to allow the passing of additional > > system properties to the VMs hosting the servers in order to change the > log > > level and prevent the benchmark measuring how long it takes to log > PdxType > > creation rather than actual time taken to create new PdxTypes. This PR > has > > been open for a very long time, so it's possible that the changes > regarding > > passing additional system properties to the VMs are now outdated or > > unnecessary, but the actual benchmarks themselves still have some value. > > > > The second PR, "Added benchmarks for aggregate functions" contains 16 new > > benchmarks related to aggregate OQL queries, (8 each for Partitioned and > > Replicated regions), which were added following work in that area by the > > Commons team. The build is currently marked as failing, but this is due > to > > a timeout rather than an actual build failure, as the number of > benchmarks > > added increased the total time to build beyond the currently configured > > timeout. Adding such a large number of additional benchmarks will > probably > > also noticeably increase the time it takes benchmarks to run, which bears > > consideration. > > > > I hope this helps shed some light for people who may look over those PRs. > > > > On Wed, Jan 22, 2020 at 11:36 AM Dan Smith wrote: > > > > > Hi, > > > > > > I noticed we have some old outstanding PRs for the geode-benchmarks > > > project. Are any of these things we want to merge or should we close > them > > > out? > > > > > > https://github.com/apache/geode-benchmarks/pulls > > > > > > -Dan > > > > > >
Re: Old geode-benchmark PRs
Donal, are you still looking at these? If they aren't ready to merge and not being worked on, should they be closed? On Wed, Jan 22, 2020 at 3:32 PM Donal Evans wrote: > Two of those PRs are mine, so perhaps I can give a bit of context for > people who might look at them. The oldest of the two, "Feature/Add PdxType > benchmark and additional framework flexibility" was an attempt to quantify > and maintain the improvement in performance for PdxType creation when large > numbers of PdxTypes already exist, and to allow the passing of additional > system properties to the VMs hosting the servers in order to change the log > level and prevent the benchmark measuring how long it takes to log PdxType > creation rather than actual time taken to create new PdxTypes. This PR has > been open for a very long time, so it's possible that the changes regarding > passing additional system properties to the VMs are now outdated or > unnecessary, but the actual benchmarks themselves still have some value. > > The second PR, "Added benchmarks for aggregate functions" contains 16 new > benchmarks related to aggregate OQL queries, (8 each for Partitioned and > Replicated regions), which were added following work in that area by the > Commons team. The build is currently marked as failing, but this is due to > a timeout rather than an actual build failure, as the number of benchmarks > added increased the total time to build beyond the currently configured > timeout. Adding such a large number of additional benchmarks will probably > also noticeably increase the time it takes benchmarks to run, which bears > consideration. > > I hope this helps shed some light for people who may look over those PRs. > > On Wed, Jan 22, 2020 at 11:36 AM Dan Smith wrote: > > > Hi, > > > > I noticed we have some old outstanding PRs for the geode-benchmarks > > project. Are any of these things we want to merge or should we close them > > out? > > > > https://github.com/apache/geode-benchmarks/pulls > > > > -Dan > > >
Re: Old geode-benchmark PRs
I hate to just close what’s there but it also likely needs some massaging. Additions to the benchmarks risk destabilizing the CI if the benchmark has a wide variance. I don’t have time right now to spend on tuning new benchmarks. If someone else does please step up and take it on. > On Jan 22, 2020, at 12:31 PM, Donal Evans wrote: > > Two of those PRs are mine, so perhaps I can give a bit of context for > people who might look at them. The oldest of the two, "Feature/Add PdxType > benchmark and additional framework flexibility" was an attempt to quantify > and maintain the improvement in performance for PdxType creation when large > numbers of PdxTypes already exist, and to allow the passing of additional > system properties to the VMs hosting the servers in order to change the log > level and prevent the benchmark measuring how long it takes to log PdxType > creation rather than actual time taken to create new PdxTypes. This PR has > been open for a very long time, so it's possible that the changes regarding > passing additional system properties to the VMs are now outdated or > unnecessary, but the actual benchmarks themselves still have some value. > > The second PR, "Added benchmarks for aggregate functions" contains 16 new > benchmarks related to aggregate OQL queries, (8 each for Partitioned and > Replicated regions), which were added following work in that area by the > Commons team. The build is currently marked as failing, but this is due to > a timeout rather than an actual build failure, as the number of benchmarks > added increased the total time to build beyond the currently configured > timeout. Adding such a large number of additional benchmarks will probably > also noticeably increase the time it takes benchmarks to run, which bears > consideration. > > I hope this helps shed some light for people who may look over those PRs. > > On Wed, Jan 22, 2020 at 11:36 AM Dan Smith wrote: > >> Hi, >> >> I noticed we have some old outstanding PRs for the geode-benchmarks >> project. Are any of these things we want to merge or should we close them >> out? >> >> https://github.com/apache/geode-benchmarks/pulls >> >> -Dan >>
Re: Old geode-benchmark PRs
Two of those PRs are mine, so perhaps I can give a bit of context for people who might look at them. The oldest of the two, "Feature/Add PdxType benchmark and additional framework flexibility" was an attempt to quantify and maintain the improvement in performance for PdxType creation when large numbers of PdxTypes already exist, and to allow the passing of additional system properties to the VMs hosting the servers in order to change the log level and prevent the benchmark measuring how long it takes to log PdxType creation rather than actual time taken to create new PdxTypes. This PR has been open for a very long time, so it's possible that the changes regarding passing additional system properties to the VMs are now outdated or unnecessary, but the actual benchmarks themselves still have some value. The second PR, "Added benchmarks for aggregate functions" contains 16 new benchmarks related to aggregate OQL queries, (8 each for Partitioned and Replicated regions), which were added following work in that area by the Commons team. The build is currently marked as failing, but this is due to a timeout rather than an actual build failure, as the number of benchmarks added increased the total time to build beyond the currently configured timeout. Adding such a large number of additional benchmarks will probably also noticeably increase the time it takes benchmarks to run, which bears consideration. I hope this helps shed some light for people who may look over those PRs. On Wed, Jan 22, 2020 at 11:36 AM Dan Smith wrote: > Hi, > > I noticed we have some old outstanding PRs for the geode-benchmarks > project. Are any of these things we want to merge or should we close them > out? > > https://github.com/apache/geode-benchmarks/pulls > > -Dan >