Re: [GitHub] renctan sent you a message

[Pratik Daga]

Yeah I did that!! actually!!
But I think I got bigger bug in a process not in my code but in node.js
If I am not wrong.

Here is the scenario

Assume I have list of servers in ServerCluster Object.

a = new ServerCluster(Serverlist);
=> d = new db("testdb", a,....)

=> than I do sleep(50)// which do sleep for 50 sec in meanwhile I put server down and up again before it goes to next line
// I do this to get new master

=> d.open(err, reply){ // here it give me err that Connection is Reset... in node.js line no....
db.close();
}

it means that node.js don't except connection reset or server up/down in between.

It should not do this because db.open() check the above situation and have code to handle it .
And I am also using same logic in my code.
So until and unless I don't fix this or find the problem in code. Doesn't make any sense

Am I clear with problem?

----- Original Message -----
From: Kristina Chodorow <kris...@10gen.com>
Date: Monday, November 8, 2010 4:57 pm
Subject: Re: [GitHub] renctan sent you a message
To: mongo-node...@googlegroups.com

> Any update on removing the whitespace and global variables?
>
>
> On Wed, Nov 3, 2010 at 2:00 PM, Pratik Daga <pd...@nyu.edu> wrote:
>
> > It seems I need to clean up a lot, let me do it
> > also let me know how to change author name as my pc is own my
> friends name
> >
> > @rectan thanks for such a great review and suggestion
> >
> >
> > ----- Original Message -----
> > From: GitHub <nor...@github.com>
> > Date: Wednesday, November 3, 2010 11:42 am
> > Subject: [GitHub] renctan sent you a message
> > To: pd...@nyu.edu
> >
> >
> > > renctan sent you a message.
> > >
> > > --------------------
> > > Subject: Comments on Pull Request
> > >
> > > First of all, I would like to thank you for devoting your time for
> > > this project. You did a great job in submitting the code. Please allow
> > > me to make some comments on your code:
> > >
> > > Please cleanup your code - remove codes that you end up not using.
> For
> > > example, in query.js, the ReplSetServers you added does not seem
> to be
> > > used. Same thing for Db and Admin for connection.js.
> > >
> > > connection.js:
> > > 1. Do you intend to make serverlist a member variable of
> > > connection.js? Because the way it is right now, it behaves more
> like a
> > > static variable (in Java, C++ terminology). Notice that the serverlist
> > > variable will be overwritten with a new variable everytime the
> > > ReplSetServers constructor is called. If you want to make it behave
> > > like a member variable, you should put them inside a method of the
> > > object you intend it to belong to, for example:
> > >
> > > x.prototype.doThis = function () {
> > > this.serverlist = null; //this is a member variable of an instance
> of
> > > x
> > > };
> > >
> > > Yeah, I know, it's weird if you are used to languages like Java &
> C++,
> > > but you'll become used to it. I think most scripting languages use
> > > that idiom as Python and Ruby also use that idiom to create it's own
> > > "instance member variables".
> > >
> > > Nevertheless, you might not need serverlist variable as it appears
> > > that you are just passing it to the ServerCluster constructor.
> > >
> > > 2. Where is serverlistcopy being used?
> > >
> > > db.js:
> > > 1. Same comment on serverlist. Did you intend to use them as member
> > variables?
> > > 2. Same comment on dbcopy as behaving like static variable. I think
> > > you may not need dbcopy if you modify checkMaster to either:
> > >
> > > //make it accept a db instance
> > > var checkMaster = function (db) {
> > > //replace previous instances of dbcopy with db
> > > };
> > >
> > > and to use it at line 543:
> > > checkMaster(this);
> > >
> > > Alternatively, you can make also checkMaster be a method of db:
> > > Db.prototype.checkMaster = function () {
> > > //replace previous instances of dbcopy with this
> > > };
> > >
> > > and to use it at line 543:
> > > checkMaster();
> > >
> > > Which approach to use depends on your intent. I am also pretty much
> > > new to js, so I am not sure of the conventions used in js, but to
> me,
> > > the second approach would expose the method to the clients, so it
> > > seems to behave like public, while the first approach is not visible,
> > > so it acts more like a private method.
> > >
> > > Great job!
> > > --------------------
> > >
> > > Reply on GitHub: http://github.com/inbox/2533177#reply
> >

[Pratik Daga]

I am trying to figure out everything into detail.
Once I solve this problem, I am fine to push code.

[Randolph]

I don't fully understand the problem, but I think we can address this
as a seperate issue as long as it is not a bug introduced by the new
code addition.

[Pratik Daga]

I will send a code which will describe code problem in sometime.

[Pratik Daga]

ok , so I am trying to figure out, why I am getting error and why it is not handled in try catch.
I am using code in example folder for testing but all of them asynchronous and if I try to add some kind of
sleep method for few seconds and try to up/down server to check functionality I am not able trace it.

Even I am not sure that is it bug in my code or is it something related to asynch.

Note:-- problem comes especially when error is thrown from node.js its just not handled by my code, it get return
------------ some error-----------
node.js:63
throw e;
^
Error: EPIPE, Broken pipe
at Stream._writeImpl (net:300:14)
at Stream._writeOut (net:732:25)
at Stream.write (net:665:17)
-------------------------------------------------

node.js:63
throw e;
^
Error: ENOTCONN, Socket is not connected
at Stream._shutdownImpl (net:309:18)
at Stream._shutdown (net:1003:14)
at Stream.flush (net:790:12)
at Stream.end (net:1029:12)
------------------------------------------------

node.js:63
throw e;
^
Error: ECONNRESET, Connection reset by peer
at Stream._readImpl (net:304:14)
at IOWatcher.callback (net:454:24)
at node.js:768:9

[Kristina Chodorow]

The main thing is to have the driver autodetect the master.  Please commit once you have that implemented.  We can all work on these little things, especially once we can see the code.

Those errors are all various networking errors caused by reading from or writing to an invalid socket (when you bring down the server, the socket becomes invalid).  Some of these should be caught and retried by the driver (e.g., reads), others are unsafe to retry (database commands, writes). 

[Pratik Daga]

Ok If that is a case I will commit it today with clean code.
Thanks.

[Kristina Chodorow]

Great!

[Pratik Daga]

Let me know what you think about code I added!!

----- Original Message -----
From: Kristina Chodorow <kris...@10gen.com>
Date: Tuesday, November 9, 2010 3:46 pm
Subject: Re: replset
To: mongo-node...@googlegroups.com

[Kristina Chodorow]

Better, but it looks like you're still using dbcopy.  You can't have any static/global variables like that, in case people are using multiple Connection instances.

[Pratik Daga]

thanks for replying,

https://github.com/pdaga/node-mongodb-native/blob/9e1f9b83f2806a05b14d18d5998455158ad6c17d/lib/mongodb/db.js

I don't think so, if you see executecommand function in db.js @ line 532
I called checkMaster(this)

and in checkMaster I used dbcopy as a parameter and it returns it when you get err or master(line 590 and 596)
and at line 532 dbcopy is handled as dbinstance

Also I haven't declared dbcopy anywhere in db.js apart from ensureMaster function
so its local to ensureMaster.

so how does it makes it global or static global.

[Kristina Chodorow]

Ah, I see!  I think that's almost exactly correct, will discuss a slight change tonight.

[Pratik Daga]

sorry for getting back late! was working on deadlines.
Will fix code as we discussed including indentation and private method
also will be working on gridfs streaming part. will get back to you by
monday.

pratik daga

[Kristina Chodorow]

Replica set code has been merged in! 

Now we have to try to break it.  We can brainstorm cases/tests on Monday and come up with more stuff to build on it.

Thanks, Pratik!

[Pratik Daga]

sure, as you showed one test case as I remember in which you have try catch in a loop.
will try to work on same approach!!