trafficserver-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jason Kenny <jke...@yahoo-inc.com.INVALID>
Subject Re: Proposal for how to update source code layout.
Date Wed, 27 Jan 2016 19:33:05 GMT
Thanks for the information.

I understand there is a pull to have a miniumn change possible we can have. If that is the
case we can look at this solution instead. 

Some of your points are unclear to me. For example what do you really mean by putting the
source under src/ is good, but bad? your point 1)

As I understand it, one argument is to fix the source. this mean such a diff will included
moved, split and refactored code and build files. The idea is that this might be a minimum
set of fixes to be good enough to deal with some issues I have raised.

what I am suggesting is to restructure to solve a number of issues and to make it clearer
what is broken. My diff would be a move and small tweak to headers file include, and tweaks
to build files. I don't try to solve all the problems,I just try to make it easier to solve
the other problems. This is not a massive change, but lots of small trackable changes... ideally
one change for easy modules we build ( which is about 35 when you count traffic_server and
everything it depends on to be built 40+ when you everything in the suite).

Git will track all my move changes correctly. Git will not track code refactoring correctly
as code will move to new files ( not moved one). because of this I would like to make this
a separate step, and done in a way to make it easier for devs to deal with the change vs what
is suggested which will be more upsetting for fixes in flux. It is a lot easier to split a
file in a modules, vs split a file and move it when tracking changes. That is just a fact.

I think the problem with the argument with making the code clean first is that the code is
worse than anyone really understands. I don't believe that the option of just fixing the miniumn
issue will result in a cleaner or easier to manage than what I suggest. I am pretty sure this
will be more complex overall for everyone, given the git tracking issues i talked about already.
I am also a little worried about the tyranny of the infrastructure argument that seems to
be coming up, which is to say that moving the source will mean a lot of moves in git someone
is worried about seeing all these changes, so lets find a way not see them. This is the same
argument to make for putting all the source in one big file as it easier than having to do
the correct updated to the build, etc to make the code more manageable and maintainable. If
the idea some would rather have is to not make a src and test directory and keep everything
jumbled as is...that is a solution. I for one disagree with it. We use tools to get stuff
done, there is no perfect tool, and any quirk in a tool should not prevent good ideas from
happening.

Given work I have done on larger more complex application than ATS, I have found that one
great way to help solve issues like these is take all the code modules and dangle them out
as equals and refactor from there. That is all I am suggesting here. Such a change I have
seen will make it easier for different people to refactor at the same time with less risk
of stepping on each other toes.

While we will probably differ on how to fix items, I think it is best I add more data. 

Below is the a general complete depends mapping of every .a/.so we make under traffic_sever.
I add a "new" module call proxy.core ( which is the source directly under proxy/.) This allows
me to say what traffic_server really needs in a clearer way.

General format is:
directory:
    module name:
          info..

The note are generally light, but should show what needs what. Keep in mind that the macro
high level depends flow should be cmd -> mgmt -> proxy -> iocore -> lib. I don't
make out depends that break this flow as "broken" below, as we can untangle the component
tree to address these issues as I suggest


Libs:
               Atscppapi:
                              Depends:
                                             Tsutil
                                             Proxy.api
               Luajit:
                              Depends:
                                             None
               Records:
                              Depends:
                                             Tsutil
                                             Mgmt.api
                              Broken Depends:
                                             Eventsystem
                                             Proxy.core (processmanager.h)
                                            
               Tsutil:
                              Depends:
                                             None
 
Iocore:
               AIO:
                              Depends:
                                             Iocore.eventsystem
               Cache:
                              Depends:
                                             Lib.tsutil
                                             Iocore.eventsystem
                                             Iocore.aio
                                             Proxy.api
                                             Proxy.hdrs
                                            
                              Broken Depends:
                                             Proxy.http (httptransactCache.h in Inline.cc)
                                             Iocore.net (see cluster)
                                             Iocore.cluster (P_CacheInternals.h line 1104
.. Want to make this a call back)
               Cluster:
                              Depends:
                                             Lib.tsutil
                                             Iocore.eventsystem
                                             Iocore.utils
                                             Iocore.net
                                             Iocore.cache
                                            
               Dns:
                              Depends:
                                             Lib.Tsutils
                                             Iocore.eventsystem
                              Broken Depends:
                                             Hostdb
                                             Net (eventIO class in net)
                                            
                                            
               Eventsystem:
                              Depends:
                                             Lib.tsutil
                                             Lib.records
               hostDB:
                              Depends:
                                             Lib.tsutil
                                             Iocore.net
                                             Iocore.dns
                                             Iocore.cache
                                             Iocore.cluster
                                            
               Net:
                              Depends:
                                             Lib.Tsutil
                                             Iocore.eventsystem
                                             Iocore.aio
                                             Proxy.logging
                              Broken Depends:
                                             Proxy.core - ParentSelection.h
                                             Iocore.dns - p_UnixNet.h ->EventIO::Start/Close..
               Util:
                              Depends:
                                             Iocore.eventsystem   
Mgmt:
               Api:
                              Depends:
                                             None
               Mgmtapi:
                              Depends:
                                             Lib.tsutil
                                             Mgmt.utils
               Mgmtapilocal:
                              Depends:
                                             Lib.tsutil
                                             Mgmt.utils
              
               Tsmgmt:
                              Depends:
                                             Lib.tsutil
                                             Mgmt.utils
                                             Mgmt.api
               Cluster
                              Depends:
                                             Lib.tsutil
                                             Mgmt.utils
                                             Lib.records
               Utils:
                              Depends:
                                             Lib.tsutil
                                             Lib.records
                              Broken Depends:
                                             Mgmt.mgmt_lm
               Mgmt (root container part)
               Mgmt_c ( not used in traffic server?)
               Mgmt_lm
                              Depends:
                                             Lib.Tsutils
                                             Lib.records
                                             Mgmt.web2
               Mgmt_p
                              Depends:
                                             Lib.Tsutils
                                             Lib.records
                                             Proxy.core (internal … InkAPIInternal.h)
                                             Mgmt.mgmt_lm
               Web2:
                              Depends:
                                             Lib.tsutils
                                             Mgmt.utils
                                             Mgmt.tsmgmt
                                             Mgmt.mgmtapilocal
                                             Proxy.hdrs
 
Proxy:
               API:
                              Depends:
                                             NONE!
               Core:
                              Depends:
                                             Lib.tsutil
                                             Iocore.eventsystem
                                             Proxy.hdrs
                                             Proxy.api
                                             Proxy.error
                                             Proxy.http
                                             Proxy.http_remap
                              Broken Depends:
                                             Mgmt.mgmt_p ProcessManger.h and ProxyConfig.h
                             
               Congest:
                              Depends:
                                             Lib.tsutil
                                             Iocore.net
                                             Proxy.core
                              Broken Depends:
                                             Mgmt.mgmt_p (proxyConfig.h)
               Diagsconfig:
                              Depends:
                                             Lib.tsutil
                                             Lib.records
               Logcollation:
                              Depends:
                                             Lib.tsutil
                                             Lib.eventsystem
                                             Proxy.logging
                                             Iocore.net
                                             Iocore.hostDB
               Logging:
                              Depends:
                                             Lib.Tsutil
                                             Lib.Records
                                             Proxy.xml
                                             Proxy.hdrs
                              Broken Depends:
                                             Proxy.core ?
                                             Proxy.http (LogAccessHttp.cc)
                                             Mgmt.mgmt_p (LogConfig.h -> ProxyConfig.h)
                                             Proxy.logcollation (LogConfig.cc) and link order
               Hdrs:
                              Depends:
                                             Libtsutil
                                             Iocore.eventsystem (hdrheap.cc like to fix this)
               http:
                              Depends:
                                             Proxy.http2 ( needed for http2ClientSessionAllocator.
Needs to be first)
                                             Lib.tsutil,
                                             Iocore.hostdb
                                             Iocore.cache (HttpCacheSM.h)
                                             Proxy.logging
                                             Proxy.spdy (sdpySessionAccept)
                                            
                              Bad depends:
                                             Http_remap - httpsm.h->HttpTransact.h needs
RemapPlugInfo.h
                                             Proxy.core - protocalProbeSessionAccept ?? (
other direction is based on state machine stuff)
               Http2:
                              Depends:
                                             Lib.tsutils
                                             Proxy.hdrs
                                             Proxy.api
                                             Iocore.net
                                             Proxy.http
               Http_remap:
                              Depends:
                                             libtsutil
                                             Proxy.core
                                             Proxy.api
                                             Proxy.hdrs
                                             Proxy.http
                                             Proxy.error
                                            
               Spdy:
                              Depends:
                                             Iocore.net
                                             proxy.error
  
Traffic_server:
               Depends:
                              Lib.tsutil
                              Iocore.eventsystem
                              Proxy.core
                              Proxy.congest
                              Proxy.error
                              Proxy.diagsconfig
                              Iocore.net
                              Proxy.logcollation
                              Proxy.logging
                              Mgmt.mgmt_p
                              Iocore.cache
  



I hope this helps in the discussion.
Jason




----- Original Message -----
From: Yongming Zhao <ming.zym@gmail.com>
To: dev@trafficserver.apache.org; Jason Kenny <jkenny@yahoo-inc.com>
Cc: Alan Carroll <solidwallofcode@yahoo-inc.com>
Sent: Wednesday, January 27, 2016 11:42 AM
Subject: Re: Proposal for how to update source code layout.

this thread is a big evolution change in TS if it happen, glad to see the discuss right now
other than change in codes.
some of my points:
1, put all codes in /src/ is good idea, but move all codes into src is a very bad idea. it
just don’t solve the complex, and add more complex into tracking the change history.
2, cut the complex means we should avoid more headache change for current developer and future
developer, and the current make choices.
3, if you want a clean and clear code base, you need to make the code clean first, +1 to James
on the dependent cleanup effort.
4, better to do more discuss and make some RFC on the short and long time plan|change.


the code change we know:
1, in Inktomi or early Yahoo age, the /mgmt/ /iocore/ bump from /proxy 
2, after open sourced, we change from the static linking into dynamic linking.
3, we removed the WEBUI and many other functions, and pending to remove some more.
4, we make new features and functions to support cache<->proxy interacting.
5, some reconstruct on layout such as binaries main file moved to /cmd/

on the detail, looks like the layout problem that make everybody crazy, the problem in your
case based on:
1, the layout of /mgmt/ mess in traffic_server traffic_manager 
  the mgmt will be very complex because we need it in both server and manager, but it works
not the same code base, that is the root cause of the complex.
my suggestion: make mgmt a lib or iocore module and split the codes into _server and _manager
_top

2, /iocore/ mess with the /proxy/
  yes, this strange, but it is there. this is the problem happened in the open sourced time,
maybe we should take a look of this a cut the iocore from depends on /proxy

3, some file dependents like mess.
  sure, we create features and we create bugs too, help find out a good fix if it bother you.


so far so mess, it always hurt during the massive change, especially in the layout. but if
you narrow it down, you may find the real problem and solve it, before we make the complete
reconstruct in the code layout.


- Yongming Zhao 赵永明


> 在 2016年1月28日,上午12:46,Jason Kenny <jkenny@yahoo-inc.com.INVALID>
写道:
> 
> I think what you are missing is that the decoupling is more complex than a few file tweaks.
There is code in the wrong place. Instead of trying to move everything around in iocore,proxy,lib,mgmt
and cmd, which will be only confusing when looking at different version of ATS, is to make
a clean separation to something clean and simple. Honestly no one seems know what current
and modern value there is in the code in mgmt, vs that of proxy, etc. We have mgmt_p dependent
on proxy code compiled into traffic_server and different code in traffic_server dependent
on mgmt_p. There are lots of these, and yes we could try a miniumn take on the changes. However
this does not solve other issue on how we scale, manage new code, remove old in a understandable
and easy way. I gave a talk on this for a reason, it is a mess. Let clean it up. You talk
about these great items to clean up, however the bar of entry for many of these are higher
than need to be, because of the lack of a simple, understandable layout, and the technical
debt that has happen to the code in the form of dependency cycles, and items such as the I_
and P_ headers being used incorrectly, mega files that need to be broken up ( but are not
because people don't know how to fix the build system), etc.... I am not saying the layout
I propose is perfect, only that it is better and that it sets up a better framework for making
changes in the future without massive complication people deal with now. I would like to do
these improvements now as this allows use to make all those other items easier for all of
us to address. I am sure other changes will happen, but those should be smaller and simpler
with this work done as a base.
> 
> Jason
> 
> 
> ----- Original Message -----
> From: James Peach <jpeach@apache.org>
> To: dev@trafficserver.apache.org; Alan Carroll <solidwallofcode@yahoo-inc.com>
> Sent: Tuesday, January 26, 2016 3:36 PM
> Subject: Re: Proposal for how to update source code layout.
> 
> 
>> On Jan 26, 2016, at 11:07 AM, Alan Carroll <solidwallofcode@yahoo-inc.com.INVALID>
wrote:
>> 
>> My view is that getting or at least having a target source tree that is better organized
is a big help in doing the things HRP wants to do. Certainly when I have looked at doing that
sort of cleanup, the current structure is an impediment. For example, the RPC logic needs
to be pulled out of mgmt and put in a separate library so it can be linked easily by any executable.
But where does that go? I suppose lib/rpc but it's unclear.
> 
> Sure, but we need to be really specific here in order to understand the proposal. What
exactly do you mean by the "RPC logic"? Just MgmtMarshall.cc and NetworkMessage.cc? Everything
NetworkMessage.cc depends on? Or do you mean the libmgmt API?
> 
> 
>> I have mixed feelings about the big shift vs. gradual. The former is more painful
but only for a short while. The latter drags out the pain so it's a somewhat chronic condition.
In either case, though, we'd need a final target that we are working toward.
> 
> If you understand how to decouple a dependency, then I think the best approach is to
just decouple that dependency and move on to the next. Given a specific change, we can understand
what it means and where is belongs.

Mime
View raw message