Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move eventlogs to an http endpoint #1382

Merged
merged 5 commits into from
Jun 18, 2015
Merged

move eventlogs to an http endpoint #1382

merged 5 commits into from
Jun 18, 2015

Conversation

whyrusleeping
Copy link
Member

This PR makes eventlogs not get written to disk, instead, they are thrown away unless a client is connected to the API endpoint at /logs

License: MIT
Signed-off-by: Jeromy jeromyj@gmail.com

@whyrusleeping whyrusleeping added the status/in-progress In progress label Jun 17, 2015
w.WriteHeader(200)
wnf, errs := newWriteErrNotifier(w)
eventlog.WriterGroup.AddWriter(wnf)
<-errs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clever

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these block after the client closes the request? if you get a write error to a closed HTTP connection you also don't see the error. Also, the MirrorWriter drops them as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cryptix that was intended. I assume that the only error we will encounter is the http connection being closed. The first error encountered will drop the connection from the mirrorwriter, and then this will exit. I could add a default case to the send on the write notifier to ensure that misuse of the code doesnt cause a hang

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see. Wasn't sure if there are are other errors. Sorry but I still don't see where the Writer is removed from the mirror.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the MirrorWriter loops over all writes on each write call, any writer that throws an error gets dropped from the list of writers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or rather, any writer that doesnt throw an error is kept around for another write

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaah.. there it is! Glancing over it, I assumed to pushes to messages to the filter but it recreates the writer slice every time.. that seems wasteful to be honest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah... the alternative was to keep an array of the items to be removed, wait til the write portion of the call was done, then rebuild the slice sans the dead writers. which was really ugly. I chose <32 bytes worth of wasted memory (on average) per write over ugly code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if worried about perf (which we may be, given that this is per event (and there are lots...), another option is:

// in Write()

// write to all writers, and nil out the broken ones.
for i, w := range mw.writers {
  _, err := w.Write(b)
  if err != nil {
    mw.writers[i] = nil
  }
}

// consolidate the slice
for i := 0; i < len(mw.writers); i++ {
  if mw.writers[i] != nil {
    continue
  }

  j := len(mw.writers)
  for  ; j > i; j-- {
    if mw.writers[j] != nil {
      mw.writers[i], mw.writers[j] = mw.writers[j], nil // swap
      break
    }
  }
  mw.writers = mw.writers[:j]
}

seem to work: http://play.golang.org/p/AV9UeZphgq

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that works. still a bit more complicated, but it may help.

@jbenet
Copy link
Member

jbenet commented Jun 17, 2015

@whyrusleeping this LGTM. it should have a sharness test case though to make sure it gets mounted correctly and we don't break it.

@whyrusleeping
Copy link
Member Author

@jbenet a test just to make sure theres an endpoint there? It would be rather hard to 'connect for two seconds' in a test, or something like that

MaxAgeDays: c.Log.MaxAgeDays,
}
eventlog.Configure(eventlog.OutputRotatingLogFile(rotateConf))
eventlog.Configure(eventlog.Output(eventlog.WriterGroup))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Log config code should be removed then, now or soon.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. I'll pull that out

@rht
Copy link
Contributor

rht commented Jun 17, 2015

Nice, don't forget to close #866, #904, #868, #1224.

for _, w := range mw.writers {
_, err := w.Write(b)
if err == nil {
filter = append(filter, w)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invert if, log and continue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really want to log these errors, worst case, a client connection drops.

@whyrusleeping
Copy link
Member Author

@jbenet whats with the swarm and notifications failures happening more often? I don't remember them popping up this much in the past.

@jbenet
Copy link
Member

jbenet commented Jun 17, 2015

@whyrusleeping

@jbenet a test just to make sure theres an endpoint there? It would be rather hard to 'connect for two seconds' in a test, or something like that

Yeah. if we dont test these things we're bound to have regressions. we have to test everything.

Maybe can use the pollEndpoint binary that's already there. or maybe even curl | head -n5 | grep <expected>

@jbenet whats with the swarm and notifications failures happening more often? I don't remember them popping up this much in the past.

not sure, will take a look. IIRC, its timing related. so overloaded machines will fail more often.

@jbenet jbenet mentioned this pull request Jun 18, 2015
@whyrusleeping
Copy link
Member Author

@jbenet this is an odd failure: https://travis-ci.org/ipfs/go-ipfs/jobs/67385491

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
@jbenet
Copy link
Member

jbenet commented Jun 18, 2015

@whyrusleeping yeah wat. it's totally nil. having hard time repro-ing it... looks like somewhere in the dialing both the conn and err are nil, so the flow doesn't go down. we can guard against it with: multiformats/go-multiaddr-net@b9c1a08

but we should probably get to the bottom of it someday.

@jbenet
Copy link
Member

jbenet commented Jun 18, 2015

@whyrusleeping thanks LGTM

jbenet added a commit that referenced this pull request Jun 18, 2015
move eventlogs to an http endpoint
@jbenet jbenet merged commit 370df8f into master Jun 18, 2015
@jbenet jbenet removed the status/in-progress In progress label Jun 18, 2015
@jbenet jbenet deleted the http-eventlog branch June 18, 2015 23:06
@jbenet jbenet mentioned this pull request Jun 22, 2015
55 tasks
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
move eventlogs to an http endpoint

This commit was moved from ipfs/kubo@370df8f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants