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 <[email protected]>
To: [email protected]; Jason Kenny <[email protected]>
Cc: Alan Carroll <[email protected]>
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 <[email protected]> 写道：
>
> 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 <[email protected]>
> To: [email protected]; Alan Carroll <[email protected]>
> 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
>> <[email protected]> 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.
