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 |
> |