close connections properly

[harpreet-sawhney]

Creates an additional flag isClosed to mark that the connection has been closed.
The Pick method is modified to ensure that a closed or at capacity connection is not picked

[0x6e6562]

Why is HandleError() no longer called?

[harpreet-sawhney]

If I remember it correctly execSimple() is only called from
startup()->Connect() and therefore needs not call HandleError

On Mon, Mar 31, 2014 at 9:14 AM, Ben Hood notifications@github.com wrote:

In conn.go:

@@ -248,8 +251,7 @@ func (c *Conn) execSimple(op operation) (interface{}, error) {
f, err := op.encodeFrame(c.version, nil)
f.setLength(len(f) - headerSize)
if _, err := c.conn.Write([]byte(f)); err != nil {

•   c.conn.Close()
  

•   c.cluster.HandleError(c, err, true)
  

Why is HandleError() no longer called?

Reply to this email directly or view it on GitHubhttps://github.com//pull/139/files#r11100738
.

[Zariel]

Yeah ive been thinking about this and think this is correct action as it could lead to an infinite loop of connect -> startup -> execSimple -> handleError -> connect

[Zariel]

Should isClosed be made thread safe?

[harpreet-sawhney]

Probably. but what is the worst that can happen :)

On Mon, Mar 31, 2014 at 9:49 AM, Chris Bannister
notifications@github.comwrote:

Should isClosed be made thread safe?

Reply to this email directly or view it on GitHubhttps://github.com//pull/139#issuecomment-39042862
.

[harpreet-sawhney]

Personally, I prefer if the Conn is kept in a resource pool and only put
back if it is had no errors.

Also as I mentioned earlier, in spite of the fixes in the #139 the number
of file descriptors continued to increase. I in fact went to the extent of
writing a finalizer for the Conn type but the connections kept on "leaking"
albeit at a slower pace.

Here is something I wrote for my company and it solved all the problem
https://github.com/harpreet-sawhney/gocql/commit/490dd0b3cb47202d07cb89ffae4907991f824357.
Warning: this is not complete but will give you an idea.

On Mon, Mar 31, 2014 at 10:22 AM, Harpreet Sawhney <
harpreet.sawhney@gmail.com> wrote:

Probably. but what is the worst that can happen :)

On Mon, Mar 31, 2014 at 9:49 AM, Chris Bannister <notifications@github.com

wrote:

Should isClosed be made thread safe?

Reply to this email directly or view it on GitHubhttps://github.com//pull/139#issuecomment-39042862
.

[harpreet-sawhney]

Growth in FDs stopped (3/21 04:00) when the new session code was used.

[mihasya]

Based on the Travis output, looks like some changes didn't quite make it into the PR?

# github.com/gocql/gocql
./bs.go:36: unknown ConnConfig field 'SetupTimeout' in struct literal
./session.go:310: n.qry.session.executeQuery undefined (type ISession has no field or method executeQuery)

[mihasya]

(first and foremost, I should say thank you for putting code forth for tackling this, this problem has been on my own long list of things to get to for a very long time and I'm ELATED to see movement on it 😁 😁 😁 😁 )

[harpreet-sawhney]

Have fixed the build error.

Please note the New Session code is incomplete but can be merged into the
main as it will not break anything. And can be used for basic queries.

On Tue, Apr 1, 2014 at 7:27 AM, Mikhail P notifications@github.com wrote:

(first and foremost, I should say thank you for putting code forth for
tackling this, this problem has been on my own long list of things to get
to for a very long time and I'm ELATED to see movement on it [image:
😁][image: 😁][image: 😁][image: 😁] )

Reply to this email directly or view it on GitHubhttps://github.com//pull/139#issuecomment-39137293
.

[phillipCouto]

My only concern with this is the session is managing connections. This the case with the current code and causing these issues of spaghetti code for closing connections and handling errors. I know this may solve an issue now but I think we need to move away from the session or cluster managing connections and leave that to a connection pool implementation. This way connection management is in 1 spot.

[phillipCouto]

Another thing the new session implementation code in the PR I feel is outside the scope of this particular PR. Having code that isn't complete in the master isn't a great thing either that's why we make forks and branches to submit complete units of code that work, have comments, and test cases.

[harpreet-sawhney]

Agreed on both counts.

I actually intended only to push out the connection close fixes. But being
a github noob didnt realize that my pull request will get updated if I
commit after I create a pull request.

The current session code is only cause we use in this production and needed
a quick fix. The session and the connection manger should be decoupled if
possible

On Tue, Apr 1, 2014 at 9:46 AM, Phillip Couto notifications@github.comwrote:

Another thing the new session implement action code in the PR I feel is
outside the scope of this particular PR. Having code that isn't complete in
the master isn't a great thing either that's why we make forks and branches
to submit complete units of code that work, have comments, and test cases.

Reply to this email directly or view it on GitHubhttps://github.com//pull/139#issuecomment-39151779
.

[Zariel]

If you could rebase this to remove the session changes and submit hem separately that would be great.

[harpreet-sawhney]

@Zariel This is now done.

[mihasya]

Gonna try and run this in our lab environment to see if it eradicates the unavailable errors.

[mihasya]

🤘 I had identified a query that produced that error every 5 or so requests. I can no longer get it to happen at all with this patch. Unfortunately don't have time to do anything more scientific at the moment, but it is definitely a night and day difference (as soon as I reverted to the pre-patched version, it dutifully 500'd on the first reload 😆 )

Thank you so much @harpreet-sawhney

[Zariel]

That is great news. Would you be able to share a query we can test with?

[mihasya]

May have been premature, gonna have to do the scientific thing after all. Due to a quirk in our build process, what the code I thought was running the new version was not actually, and I had just gotten lucky 😳

I don't think it's so much a particular query as query concurrency. I'll try to hook up apachebench or something similar later on.