| Message ID | 20240329143956.1602707-1-michael.opdenacker@bootlin.com |
|---|---|
| Headers | show |
| Series | prserv: add support for an "upstream" server | expand |
On Fri, 2024-03-29 at 15:39 +0100, Michael Opdenacker via lists.openembedded.org wrote: > From: Michael Opdenacker <michael.opdenacker@bootlin.com> > > This makes it possible to customize an "upstream" distribution > by modifying local packages. If the "upstream" package bears > revision "x", the local one will have revision "x.y", this > having priority over the upstream one. > > Take advantage of this work to clean-up and update the prserv code > too. > > Michael Opdenacker (12): > prserv: simplify the PRServerClient() interface > prserv: use double quotes by default > bitbake-prserv: replace deprecated optparse by argparse > prserv: use self.logger instead of logger directly > asyncrpc: include parse_address from hashserv > prserv: capitalization and spacing improvements > prserv: fix read_only test > prserv: add extra requests > prserv: remove redundant exception handler > prserv: correct error message > prserv: remove unnecessary code > prserv: add "upstream" server support Most of these series looks like cleanups to make prserv look more like hashserv, which is fine. The piece I'm struggling to understand is what happens if we have three prservs where A has B an upstream and B has C as an upstream. Does the system cope with multiple levels of increment, x.y.z and so on? I'm a bit worried we're seeing floats in the results, which suggests that generic x.y.z wouldn't work? Also, where do we stand on automated tests for this code? Cheers, Richard
Hi Richard, Many thanks for the first review! On 4/3/24 at 17:09, Richard Purdie wrote: > On Fri, 2024-03-29 at 15:39 +0100, Michael Opdenacker via > lists.openembedded.org wrote: >> From: Michael Opdenacker <michael.opdenacker@bootlin.com> >> >> This makes it possible to customize an "upstream" distribution >> by modifying local packages. If the "upstream" package bears >> revision "x", the local one will have revision "x.y", this >> having priority over the upstream one. >> >> Take advantage of this work to clean-up and update the prserv code >> too. >> >> Michael Opdenacker (12): >> prserv: simplify the PRServerClient() interface >> prserv: use double quotes by default >> bitbake-prserv: replace deprecated optparse by argparse >> prserv: use self.logger instead of logger directly >> asyncrpc: include parse_address from hashserv >> prserv: capitalization and spacing improvements >> prserv: fix read_only test >> prserv: add extra requests >> prserv: remove redundant exception handler >> prserv: correct error message >> prserv: remove unnecessary code >> prserv: add "upstream" server support > Most of these series looks like cleanups to make prserv look more like > hashserv, which is fine. > > The piece I'm struggling to understand is what happens if we have three > prservs where A has B an upstream and B has C as an upstream. Does the > system cope with multiple levels of increment, x.y.z and so on? I haven't tested that yet, but wasn't sure we wanted to support such as multi-level scenario. If you confirm that's within the scope, I can try to make this work! > > I'm a bit worried we're seeing floats in the results, which suggests > that generic x.y.z wouldn't work? I first tried to explicitly convert all the responses from the PR server to strings, but that didn't help. Could it be that a return value like '0.1' could be automatically interpreted as a float if it can be converted to one? There is no explicit conversion to float in the PR server code, so it seemed to me that the conversion was done automatically. > > Also, where do we stand on automated tests for this code? Not done yet. I was waiting for the reviews on the first series, but I understand that tests will be necessary :) Cheers Michael.
On Wed, 2024-04-03 at 17:33 +0200, Michael Opdenacker wrote: > Hi Richard, > > Many thanks for the first review! > > On 4/3/24 at 17:09, Richard Purdie wrote: > > On Fri, 2024-03-29 at 15:39 +0100, Michael Opdenacker via > > lists.openembedded.org wrote: > > > From: Michael Opdenacker <michael.opdenacker@bootlin.com> > > > > > > This makes it possible to customize an "upstream" distribution > > > by modifying local packages. If the "upstream" package bears > > > revision "x", the local one will have revision "x.y", this > > > having priority over the upstream one. > > > > > > Take advantage of this work to clean-up and update the prserv code > > > too. > > > > > > Michael Opdenacker (12): > > > prserv: simplify the PRServerClient() interface > > > prserv: use double quotes by default > > > bitbake-prserv: replace deprecated optparse by argparse > > > prserv: use self.logger instead of logger directly > > > asyncrpc: include parse_address from hashserv > > > prserv: capitalization and spacing improvements > > > prserv: fix read_only test > > > prserv: add extra requests > > > prserv: remove redundant exception handler > > > prserv: correct error message > > > prserv: remove unnecessary code > > > prserv: add "upstream" server support > > Most of these series looks like cleanups to make prserv look more like > > hashserv, which is fine. > > > > The piece I'm struggling to understand is what happens if we have three > > prservs where A has B an upstream and B has C as an upstream. Does the > > system cope with multiple levels of increment, x.y.z and so on? > > > I haven't tested that yet, but wasn't sure we wanted to support such as > multi-level scenario. If you confirm that's within the scope, I can try > to make this work! I believe we will need to support a potential hierarchy of these servers in the same way hash equivalence can. > > I'm a bit worried we're seeing floats in the results, which suggests > > that generic x.y.z wouldn't work? > > > I first tried to explicitly convert all the responses from the PR server > to strings, but that didn't help. Could it be that a return value like > '0.1' could be automatically interpreted as a float if it can be > converted to one? There is no explicit conversion to float in the PR > server code, so it seemed to me that the conversion was done automatically. I suspect that it is SQL that is making these floats and you're just passing the results around from there. The trouble is that whilst X+0.1 will work for x.y, it isn't going to work for x.y.z and beyond. That is one reason I'm mentioning this now, at same time as we're seening issues with the float() values. > > Also, where do we stand on automated tests for this code? > > > Not done yet. I was waiting for the reviews on the first series, but I > understand that tests will be necessary :) I would have a bit more confidence if there were some test cases showing what was working! Cheers, Richard
From: Michael Opdenacker <michael.opdenacker@bootlin.com> This makes it possible to customize an "upstream" distribution by modifying local packages. If the "upstream" package bears revision "x", the local one will have revision "x.y", this having priority over the upstream one. Take advantage of this work to clean-up and update the prserv code too. Michael Opdenacker (12): prserv: simplify the PRServerClient() interface prserv: use double quotes by default bitbake-prserv: replace deprecated optparse by argparse prserv: use self.logger instead of logger directly asyncrpc: include parse_address from hashserv prserv: capitalization and spacing improvements prserv: fix read_only test prserv: add extra requests prserv: remove redundant exception handler prserv: correct error message prserv: remove unnecessary code prserv: add "upstream" server support bin/bitbake-prserv | 99 ++++++++++++----- lib/bb/asyncrpc/client.py | 23 ++++ lib/hashserv/__init__.py | 27 +---- lib/prserv/__init__.py | 21 +++- lib/prserv/client.py | 42 +++++-- lib/prserv/db.py | 176 ++++++++++++++++++++--------- lib/prserv/serv.py | 228 ++++++++++++++++++++++++++++---------- 7 files changed, 434 insertions(+), 182 deletions(-)