Routes list is not in any order

pwielgolaski commented 9 years ago

There is map used for storing routes, but as it is stated here http://blog.golang.org/go-maps-in-action "Iteration order" is not guaranteed to be the same from one iteration to the next. I wonder if it makes sense to make it more under control, especially when I want to forward only to first matching.

Dieterbe commented 9 years ago

wow, you are right. this is a bug right now!

re: first_only matching, please see #23

pwielgolaski commented 9 years ago

It seems that it is quite deep in code https://github.com/BurntSushi/toml is using map so we loose information about order already when we read configuration, do you think we could change configuration from

[routes.carbon-default]
patt = ""
addr = "127.0.0.1:2005"
spool = false
pickle = false

to

[[route]]
name = "carbon-default"
patt = ""
addr = "127.0.0.1:2005"
spool = false
pickle = false

In this way we could keep array instead of map.

Dieterbe commented 9 years ago

indeed, that seems like the correct approach to fix this.

Dieterbe commented 9 years ago

@pwielgolaski I'm making good progress to refactoring the structures as part of #23. please have a look at the next branch and HACKING.txt with this new codebase, there's 1 table structure which has a list of routes, and every route has a list of endpoints

pwielgolaski commented 9 years ago

I tried to run version from /next branch but it does not work for me, init table is empty so table is not populated with routes.

I'n not convinced that it is good direction to change route definition to bespoken format of command instead of something like json. Additionally table should make public Route or give some access to it.

Dieterbe commented 9 years ago

yeah the code is in a state where everything looks more or less allright (some notable todo's left such as http ui/callbacks and non blocking tcp for the endpoints), and it compiles. but i haven't actually tried and tested it yet, so there's probably a bunch of bugs still.

the commands format was needed anyway for the tcp interface, so i figured i can just reuse it for the config. otherwise there would be more custom code needed just to transform a config spec into the populated routing table. so this was easier to get to a prototype, but this approach is not set in stone

not sure what you mean with your last comment the table currently has these functions:

func (table *Table) Snapshot() Table {
func (table *Table) GetRoute(key string) *Route {
func (table *Table) AddRoute(route *Route) {
func (table *Table) AddBlacklist(matcher *Matcher) {
func (table *Table) DelRoute(key string) error {

this seems enough to get started, no? of course we can (and probably will have to) refactor/extend some api's to serve the admin http interface better

pwielgolaski commented 9 years ago

I think that when you decided to make so big shift in implementation, we should also address issue https://github.com/graphite-ng/carbon-relay-ng/issues/4 as without persistency I don't feel that http/telnet admin is so cool. I get your point that it was easier to make prototype using command, my point was that it is not super call for human.

I'd not mind to help you to setup up HTTP interface back on track, but it would be heplfull if we have any representation ready. At the moment as I mentioned no command is parsed, so no destination is added to route.

The issue that I mentioned is that I can get from Table list of Route, I can only get one Route if I know the key. So we could have Routes public or GetRoutes.

Btw. I wonder maybe Matcher could be immutable?

Dieterbe commented 9 years ago

aha, do you mean that after making changes via telnet/http, persisting to disk could be done in a representation that can also be used as declarative configuration for the next run? that actually is a good point and makes a lot of sense to aim for a declarative config again. (note that #4 doesn't necessarily involve storing a file to disk, it can be done by inheriting in-memory datastructures, I think)

At the moment as I mentioned no command is parsed, so no destination is added to route

yeah, that code (and the other code) just needs some test runs and bugfixing.

The issue that I mentioned is that I can get from Table list of Route, I can only get one Route if I know the key. So we could have Routes public or GetRoutes.

this is what Snapshot() is for. it's like the old Copy(). it should give you a copy of the table, all its routes, and all their destinations, including matcher objects etc. (but not actually running, it's just to inspect the setup of the current routing pipeline)

Btw. I wonder maybe Matcher could be immutable?

hmm yes, perhaps that would simplify the code a bit. it would need no snapshot method and no updateRegex method. we would just switch entire Matchers when we want to make a change.

Dieterbe commented 9 years ago

hey i pushed another bunch of changes to next. see #23.

pwielgolaski commented 9 years ago

I tried today to use latest code for REST API and have few things to clarify with you. Please take a look at simple changes that I made to get some REST result form http://localhost:8081/routes and http://localhost:8081/routes/carbon-default

Check my latest commit: https://github.com/pwielgolaski/carbon-relay-ng

Issue that I noticed:

by design json enconding works on all public variables, so we should decided what in fact should be public
I change few "In" channels as json enconding does not work properly with it, I changed to "in", but we can use also notation json:"-" see http://golang.org/pkg/encoding/json/
I think in general to go for it and annotate all classes that we what to publish with it, so we dont have field starting with capital letter as it is not so json style ;)
another issue is with Matcher, json encoding is transforming byte[] into base64 string, which is fine, but we have string in fact there, so I think we have two options, make public string field and internal byte[] or we need to implement json marshalling for this class

So in short we need to define our class representation so we could use it in REST.

If you want I can create pull request from my changes.

Dieterbe commented 9 years ago

lowercasing in and regexobj looks good, the more we can use this simple method (as opposed to annotations) the better. I agree that uppercase doesn't look that good in json, so if we can make the members private let's do that, otherwise let's do annotations. Matcher's Prefix and Sub have to be []byte because they work on []byte's, it's more efficient to do all matching like this (nothing new, probably). Both of your suggested approaches sound good to me, you can do whatever you think is nicest :)

Dieterbe commented 9 years ago

fixed in master :)

grafana / carbon-relay-ng

Routes list is not in any order #22