On 07/07/2016 04:16 PM, Amos Jeffries wrote: > On 8/07/2016 7:22 a.m., Alex Rousskov wrote: >> On 07/06/2016 10:52 PM, Amos Jeffries wrote: >>> On 7/07/2016 10:24 a.m., Alex Rousskov wrote: >>>> Q4. What to do with the existing src/servers/Server.h? >>>> * It does not fully encapsulate connection-managing code of its kids >>>> (its stated usage).
>>> ... those changes to make it so got blocked in audit. >> Whether some future changes will or will not address this part of the >> problem is pretty much irrelevant to the problem existence (it only >> confirms that existence if you will). > Its pretty significant that you blocked a class being implemented to > match its design and now use the fact that its not happeed as a reason > against the class existing at all. I understand that the combination looks bad, especially if you phrase it like that, but the actual situation is far more nuanced: - I really doubt that there is some wonderful pending patch that would fully address this part of the problem. - If I block a version of a patch, there are several means to unblock progress. It is unfair to imply that I made progress impossible. - Resolving one problem in the wrong place often creates two new ones. Who knows how many more bullets would appear on this list if that wonderful change were to be committed. - Removing this bullet/problem requires adding more code to Server.h, which means more changes to undo. This bullet is about a Server.h imperfection. It is just meant to make us like that code less, so it is even easier to accept the small burden of undoing its addition. Because of the other problems I have mentioned, even a "perfect" Server.h class would need to be merged back into ConnStateData! > Our disgareement is about which class of the two is "broken". I do not think there can be any serious disagreement about that. Both classes are clearly broken, and we may even agree on what the end result should be. There can only be a disagreement regarding the correct way to get to that end result. Normally, I would resist wasting a lot of discussion time if others insist on doing things the wrong way as long as the end result is correct. In this particular case, the amount of [current and especially future] pain and suffering associated with this wrong way warrants spending more time in hope to correct the situation. > IMO a class lacking some of its needed API (::Server) is far less broken > than a class with a lot of differently scoped bits tacked on all over > the place (ConnStateData). That decision logic is valid in many cases but it is inapplicable here. We are not deciding which of the two alternative implementations of X to adopt! Instead, we have a single complex working implementation of X (including a few misplaced features that X should not be concerned with) and an essentially empty/unused X parent. Moving most of X code to its parent class to isolate those misplaced features does not solve any serious problems but does introduce new ones. The only sane solution is to remove those misplaced features from X and continue to use and improve X (under a new name). >>> The two grandchildren in trunk we planned to make direct children of it >>> (yes we, all three of us at the time agreed). The intermediate kid being >>> the ConnStateData mess still *temporarily* in the middle of the hierarchy. >> *If* I agreed on moving Server code out of ConnStateData instead of >> moving non-Server code out of ConnStateData, then I made a major mistake! >> >> What anybody did or did not say years ago is secondary to the current >> discussion, > It is the status-quo design basis on which the current discussion is > taking place. So yeah, it is relevant. Agreed. I did not say it is irrelevant. I said it is secondary. *If* we agreed on the wrong path in the past, we ought to re-evaluate our approach now. I actually believe we agreed on the same path that I am advocating now [but that is secondary]. >> but I know that in 2014 I said pretty much the same thing I >> am saying now[4]: "Keep ConnStateData as is for now, until somebody >> volunteers to extract HTTP code from it and place the extracted HTTP >> code into servers/HttpServer, moving the remaining generic code into >> src/servers/Server and src/http/". >> > > Yes, exactly. As the person who for all those years has been attemping > to *do* that work... Sorry, I am not sure what you mean. Are you implying that you are the only person who did that work? And that being the only person doing that work somehow automatically makes your work correct? Both implications are false IMO, but perhaps I am misinterpreting what you are implying by that statement. >> In other words: >> >> 1. Extract HTTP server-specific code from ConnStateData. >> >> ...once #1 is done, ConnStateData will only have generic code left... > Assuming the extraction happens and audit does not block it. Which was > my point above. Step #1 remains correct whether a specific patch is or is not blocked. No patches were blocked because they were extraction attempts. If anything was blocked, it was blocked for _other_ reasons. Mentioning those blocked-for-other-reasons patches here does not make any of us right or wrong. > I *have* been proposing exactly those changes and getting blocks thrown > up constantly preventing stuff being moved out of ConnStateData. Why are you saying that here? Does the fact that some of your changes were blocked somehow justify Server.h existence? I do not think so. If anything, it is a [very weak] vote against Server.h! I can explain or defend specific blocking decisions. I cannot explain or defend all of them at once, here, because they were all different, specific to the changes being proposed. > Usually on grounds that as a parial-measure its not good enough, The question is "Was the patch good enough to be committed?". If the correct answer is "no", then the patch was blocked correctly and mentioning that bad patch here is counterproductive to your argument. If the correct answer is "yes", then you should work on unblocking those correct changes. We are all reasonable people without absolute powers so it is possible to unblock something if it was wrongly blocked. However, this thread is the wrong place to do that because the unblocking discussion has to be specific to the change being proposed. > like > ConnStateData as a whole (huge and messy as it is) must be retained and > perfected in a single step before anything is acceptible. That requirement would be clearly absurd/indefensible. Nothing was or can be blocked based on that bogus requirement [that never existed]. >> 2. Move that remaining generic code into src/servers/Server and src/http/. >> >> We all have been doing step #1 since then, but we are not yet ready for >> step #2 because ConnStateData still has HTTP server-specific code in it. > Apart from auditing all I'm seeing from Factory is more and more > non-Server things being added to ConnStateData. First it was 1xx message > handling, then ssl-bump, then all the FTP translation logics, then > Downloader (thanks for reversing on that one). If you think that those who introduced FTP and HTTP Server child classes did not contribute enough to the ConnStateData untangling, and if you are careless or mean enough to throw in SslBump feature added five years before the untangling discussion started or Downloader that was posted as a _preview_ and did _not_ add much to ConnStateData even in that unfinished form, then I am not going to try to change your mind. Moreover, whether XYZ has contributed to ConnStateData cleanup is completely irrelevant to this discussion! >> When we are ready for step #2, the "move" will be exactly that -- we >> will be moving files and renaming classes to match their new location. >> There is no time in this plan where there are two Server classes, one >> called "Server" and one called "ConnStateData"! > The .h and .cc files are shared with ClientSocketContext and > ClientHttpRequest. Which are more or less all of BB2 between them. Yes, I know. > Since a fairly big portion of the ConnStateData logic is also BB2 scope > stuff done in the wrong class it just gets a rename in-place during > stage #1. Sorry, I do not know what you mean by "it" and "gets a rename in-place" in this context. > So in order to get any given file ready for a rename/move. We either Just to avoid misunderstanding: Renaming/moving classes is separate from renaming/moving files. Both will be done, but the decision on what to rename/move (and where) is separate for each file and each class. > a) shuffle _all_ the BB2 scoped stuff out of its currently locations > (regardless of whether its in ConnStateData or not). OR, > > b) shuffle the (relatively smaller) pieces of ConnStateData out to > Server.* during step #1 and the rename/move is to change the file into > whatever the BB2 component name is. I disagree. Doing (a) is not required to rename ConnStateData class to Server. As far as (a) and renaming are concerned, we only need to remove BB2 stuff from the ConnStateData class, and there is not much of it left, as revamped Downloader and old ESI classes prove. I will post specific suggestions after the Downloader work is completed. And option (b) appears to assume that ConnStateData class is mostly BB2, when, in fact, it is not. >>>> * It forces new code to add pure virtual methods that ConnStateData then >>>> overrides and implements, reminding me of the old Store class that >>>> slowly accumulated dozens of such methods because we needed to add >>>> something that one of the Store kids implemented. >>> This is backwards. >> Exactly! The ConnStateData split forces us to do things backwards. > No I meant your proposal is backwards. Moving a mountain instead of a > molehill. I am not moving a mountain. I am renaming it after moving a few misplaced boulders elsewhere. You, on the other hand, are splitting one mountain into two, building various temporary ladders to cross the divide while moving boulders from one side of the divide to another. Will the mountain need a split sometime in the future? We do not know yet. What we do know that the split is not needed now, and that it creates problems. >> The correct way to remove code misplaced in ConnStateData is to move >> that misplaced code (rather than to split ConnStateData in two and then >> move everything else into the second class, leaving misplaced code in >> ConnStateData, as misplaced as it was when we started). > What remains will be BB2. Perhaps we define BB2 boundaries differently, but I believe that removing BB2 from the ConnStateData class is a relatively small change. I can volunteer to make that change after Downloader work is completed. It should not take long. > You are calling to revert the changes that separate the Server logics > from the BB2+Server logics still in ConnStateData. Not really. I am essentially calling to replace the letters "Server" with the letters "ConnStateData" and move/merge ~7 simple methods currently in Server.cc. You can even keep the Server.cc file if you think that keeping those methods in a separate file is valuable. Even keeping the declarations in the Server.h file is possible if you really insist. >> I certainly regret if something I said earlier was misinterpreted as the >> need to create a Server class that slowly accumulates code already >> properly placed in ConnStateData. I hope the above arguments prove that >> we must do the opposite. Pretty much everything in design and code >> refactoring is a matter of opinion, but this choice is as close to being >> "formally provable" as it gets. > You seem to have forgotten the reason why you agreed to it after (3?) > months of discussion last time around. And what was that reason? > I dont think that was ever > written down exactly so can't point at it, but I do remember you > agreeing to it early one morning for me. Sounds like a "you agreed to it then, so it must be right, even though it certainly looks wrong now, and we do not know why you agreed to it in the first place" argument. I do not think that argument is good enough here: We are not lawyers arguing whether the plaintiff has signed a piece of paper. Our decisions should be based primarily on merits, not protocol (and the protocol part also works against you -- I just do not want to play that part of the game!). > I certainly regret that you think anything at all of the current logic > should be retained in ConnStateData. I do not think that. There are known/marked HTTP- and FTP-specific pieces in ConnStateData that should be moved. There are a couple of BB2 pieces as well. And other stuff, I am sure (this is not a comprehensive list). > Whichever way we go what the ::Server needs is: ... Snipped to avoid discussing complex design issues irrelevant for this thread. I agree with some of the items you listed, but not necessarily all of it, and there may be missing items... > Some things to note about that list vs where we are today: > > * the design of ::Server presenting generic I/O does not exist in > ConnStateData. Never has. What it has is logics for Http1::Server child > to read and write directly to the socket. That is making all the child > protocols except HTTP/1 more complicated than they should be. I do not follow. Nobody is currently proposing to delete the "generic" I/O code in Server.cc or its interface in Server.h. ConnStateData does not even override those methods so the interface will continue to be used as is and the methods will require virtually no modifications to move them back to the ConnStateData class. > * that ssl-bump, PROXY protocol, HTTP/1 vs h2 vs FTP, HTTP/1 1XX status > handling etc are all BB2 or Server-child specific activities. The child > duty in some places may be simplified down to passing an appropriate > Parser/Packer object for the ::Server to use via its Parser/Packable API > - but that is still child specific. I may disagree about some of it but it is not important right now IMO. > We dont need to go near the existing Server vs ConnStateData separation > to do that shuffling. In fact ConnStateData is acting somewhat as a TODO > list of things yet to be shuffled. Here is how your argument/plan looks from my side: "Ignore the problems and pains associated with having two Server classes that you have extensively documented. Let me commit lots of changes that you have strongly disagreed with in the past. Silently suffer for a few years until I complete the complex work still ahead. Then you will enjoy the 'perfect Server class' nirvana." Needless to say, "move a few methods back to ConnStateData" sounds like a much better plan to me! Please note that my plan does not prevent you from accomplishing everything you want to accomplish and does not really increase your suffering by much -- you may still treat those few previously identified by you methods as "true Server" methods, use them as needed, and even add more to their collection. Thank you, Alex. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
