Gentoo Archives: gentoo-portage-dev

From: Alec Warner <antarus@g.o>
To: gentoo-portage-dev@l.g.o
Cc: Zac Medico <zmedico@g.o>
Subject: Re: [gentoo-portage-dev] [PATCH 0/4] rsync: add key refresh retry (bug 649276)
Date: Sun, 01 Apr 2018 19:38:52
Message-Id: CAAr7Pr8E=UUB2QYjhnvDEfeVMihCQyQbGMQ-CvFqmBvVOvs=QQ@mail.gmail.com
In Reply to: Re: [gentoo-portage-dev] [PATCH 0/4] rsync: add key refresh retry (bug 649276) by "Michał Górny"
1 On Sun, Apr 1, 2018 at 2:29 PM, Michał Górny <mgorny@g.o> wrote:
2
3 > W dniu nie, 01.04.2018 o godzinie 08∶59 -0700, użytkownik Zac Medico
4 > napisał:
5 > > On 04/01/2018 03:57 AM, Michał Górny wrote:
6 > > > W dniu sob, 31.03.2018 o godzinie 19∶46 -0700, użytkownik Zac Medico
7 > > > napisał:
8 > > > > Since key refresh is prone to failure, retry using exponential
9 > > > > backoff with random jitter. This adds the following sync-openpgp-*
10 > > > > configuration settings:
11 > > > >
12 > > > > sync-openpgp-key-refresh-retry-count = 40
13 > > > >
14 > > > > Maximum number of times to retry key refresh if it fails. Between
15 > > > > each key refresh attempt, there is an exponential delay with a
16 > > > > constant multiplier and a uniform random multiplier between 0 and
17 > 1.
18 > > > >
19 > > > > sync-openpgp-key-refresh-retry-delay-exp-base = 2
20 > > > >
21 > > > > The base of the exponential expression. The exponent is the number
22 > > > > of previous refresh attempts.
23 > > > >
24 > > > > sync-openpgp-key-refresh-retry-delay-max = 60
25 > > > >
26 > > > > Maximum delay between each retry attempt, in units of seconds.
27 > This
28 > > > > places a limit on the length of the exponential delay.
29 > > > >
30 > > > > sync-openpgp-key-refresh-retry-delay-mult = 4
31 > > > >
32 > > > > Multiplier for the exponential delay.
33 > > > >
34 > > > > sync-openpgp-key-refresh-retry-overall-timeout = 1200
35 > > > >
36 > > > > Combined time limit for all refresh attempts, in units of seconds.
37 > > > >
38 > > > > Bug: https://bugs.gentoo.org/649276
39 > > > >
40 > > > > Zac Medico (4):
41 > > > > Add ForkExecutor (bug 649588)
42 > > > > Add ExponentialBackoff and RandomExponentialBackoff
43 > > > > Add retry decorator (API inspired by tenacity)
44 > > > > rsync: add key refresh retry (bug 649276)
45 > > > >
46 > > > > cnf/repos.conf | 5 +
47 > > > > man/portage.5 | 19 +++
48 > > > > pym/portage/repository/config.py | 22 ++++
49 > > > > pym/portage/sync/modules/rsync/rsync.py | 16 ++-
50 > > > > pym/portage/sync/syncbase.py | 85 +++++++++++-
51 > > > > pym/portage/tests/util/futures/test_retry.py | 147
52 > +++++++++++++++++++++
53 > > > > pym/portage/util/_eventloop/EventLoop.py | 45 ++++++-
54 > > > > pym/portage/util/backoff.py | 48 +++++++
55 > > > > pym/portage/util/futures/executor/__init__.py | 0
56 > > > > pym/portage/util/futures/executor/fork.py | 130
57 > +++++++++++++++++++
58 > > > > pym/portage/util/futures/futures.py | 6 +
59 > > > > pym/portage/util/futures/retry.py | 178
60 > ++++++++++++++++++++++++++
61 > > > > 12 files changed, 697 insertions(+), 4 deletions(-)
62 > > > > create mode 100644 pym/portage/tests/util/futures/test_retry.py
63 > > > > create mode 100644 pym/portage/util/backoff.py
64 > > > > create mode 100644 pym/portage/util/futures/executor/__init__.py
65 > > > > create mode 100644 pym/portage/util/futures/executor/fork.py
66 > > > > create mode 100644 pym/portage/util/futures/retry.py
67 > > > >
68 > > >
69 > > > This essentially looks like ~700 lines of code to try to workaround
70 > > > broken networking. I would rather try to do that using 5 lines of code
71 > > > but that's just me, and my programs aren't enterprise quality. I just
72 > > > hope it actually solves as many problems as it's going to introduce.
73 > >
74 > > The vast majority of this code is generic and reusable, and I do intend
75 > > to reuse it. For example, the executor support will be an essential
76 > > piece for the asyncio.AbstractEventLoop for bug 649588.
77 >
78 > Sure it is and sure you will. But tell me: who is going to maintain it
79 > all? Because as far as I can see, we're still dealing with a bus factor
80 > of one and all you're doing is making it worse. More code, more
81 > complexity, more creeping featurism and hacks.
82 >
83
84 I'll split this reply into two sorts of points. One is a brief point about
85 this patchset. The other is a larger point about the project and its
86 direction.
87
88 About this patchset:
89
90 I don't mind this code because its not domain specific and I can read it
91 and understand it. Many apps need async ways to run code, and anyone who
92 has used the futures API will find this code familiar. I reviewed it in
93 about 10 minutes. I'm not heavily invested with the async-wagon that Zac is
94 driving, but I also see how its an area where performance can be improved
95 by doing more stuff at once using an async operations. I think if you have
96 more generic concerns with the async frameworks I'd love to see some
97 discussion about them (particularly around how we can improve perf by using
98 tighter algorithms; often the 'perf' boost is just higher CPU utilization
99 and driving multi-core systems which results in better walltime perf, but
100 not necessarily cpu-perf.)
101
102 Its fine to disagree here. I think the project has some requirements around
103 minimized dependencies, so the choice is either to pull in some kind of
104 retry-lib and add a dep, or add this code. Its also not immediately clear
105 if the 3rd party deps would still not require some glue code to work with
106 our async loops.
107
108
109 >
110 > Last time you went away, you left us with a horribly unmaintainable
111 > package manager full of complexity, hacks and creeping featurism
112 > and a Portage team whose members had barely any knowledge of the code.
113 > Just when things started moving again, you came back and we're back to
114 > square one.
115 >
116
117 > Today Portage once again is a one-developer project, full of more
118 > complexity, more hacks and more creeping featurism. And we once again
119 > have a bus factor of one -- one developer who apparently knows
120 > everything, does everything and tries to be nice to everyone, except he
121 > really ignores others, makes a lot of empty promises and consistently
122 > makes the health of the project go from bad to worse.
123 >
124
125 > So, please tell me: what happens when you leave again? How have you used
126 > your time in the project? What have you done to make sure that
127 > the project stays alive without you? Because as far as I can see, adding
128 > few thousand of lines of practically unreviewed code every month does
129 > not help with that.
130 >
131
132 About the project:
133
134 I'm not really sure if you have had conversations 1:1 or on the dev-portage
135 alias (I've been on it for a month or two, but was off it for a while)
136 but in general I haven't seen these conversations (and FWIW I'd appreciate
137 if it there were on this list, so thanks for sending this email.)
138
139 I'll try to cover a few points:
140
141 1) Unreviewed code. I think in general Zac posts things to the list and
142 there is an opportunity for review. No one appears to be around to review
143 the code (most code goes unreviewed; is my observation.) I don't think Zac
144 not posting indicates a manner in which he is trying to 'sneak code by'; as
145 this is unnecessary given the current structure. If you want more review,
146 we need people to review the code. I'm fairly sure you have encountered
147 similar when you post patchsets to the list and hear little back. One
148 common problem when things are mailed to the list is that there is no
149 explicit owner of the review, so the review never occurs. We could try a
150 few things like:
151
152 a) Have an explicit coding buddy; they are responsible for reviewing code
153 for specific bugs / features?
154 b) Get folks to signup for a rotation where they commit some time (e.g.
155 1h / w) on being the 'point person' for portage code reviews. So all
156 reviews to the list would get 'assigned' to someone.
157
158 In addition, I'd encourage people to 'ack' code that looks good; making
159 reviews explicitly ack'd is helpful, as opposed to just waiting for some
160 timeout period (e.g. "well no one said no to my code after 7 days, so I'll
161 merge it"). I'd encourage 'active' reviews, where only 'good' code gets in,
162 as opposed to mediocre code that no one bothered to review.
163
164 2) Feature-ism. I think this is the most contentious point. Ultimately your
165 goal is to see portage perhaps bench feature development for a while and
166 work on paying down some technical debt. But in practice that isn't
167 happening. I'd like to learn more about why.
168
169 a) Is Zac rejecting refactoring patches?
170 b) Is Zac not working on refactoring (and to be clear, I don't see
171 'forcing' him to do this to be useful expenditure of effort.).
172
173 3) Zac disagrees with you on specific patchsets. E.g. the implementation of
174 INSTALL_MASK seemed like there were some contentious points. One thing you
175 can do is try to actually get support from other contributors to try to
176 support your point. I think you will need to be ready to compromise on
177 various issues though.
178
179
180 > I forked Portage because I didn't want to fight with you. When I forked
181 > it, I declared that I will merge mainline changes regularly for
182 > the benefit of my users. But after a week, I really start feeling like
183 > that's going to be a really bad idea. Like it's time to forget about
184 > mainline Portage as a completely dead end, and go our separate ways.
185 >
186
187 I would consider thinking about this more. If a week sways you I'm somewhat
188 concerned how committed you were to merging changes to begin with.
189 Ultimately you not "fighting" with Zac means he will merge whatever (which
190 you already said you didn't want) and then you merging it into
191 portage[mgorny]. So if that was the extent of your plan I don't see how it
192 could end otherwise.
193
194
195 > I'm seriously worried about the future of Gentoo. I'd really appreciate
196 > if you started focusing on that as well. I get that all this stuff looks
197 > cool on paper but few months or years from now, someone will curse
198 > 'whoever wrote that code' while trying to fix some nasty bug. Or get
199 > things moving forward. Or implement EAPI 8.
200 >
201
202 I appreciate having this conversation.
203
204
205 >
206 > --
207 > Best regards,
208 > Michał Górny
209 >
210 >
211 >