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

Getting configuration error when importing data through a parent module #77

Closed
stephenlacy opened this issue Feb 1, 2018 · 8 comments · Fixed by #109
Closed

Getting configuration error when importing data through a parent module #77

stephenlacy opened this issue Feb 1, 2018 · 8 comments · Fixed by #109

Comments

@stephenlacy
Copy link

I'm setting up to import the geonames data into my elasticsearch cluster, and am hitting this error when running npm start:

/root/geonames/node_modules/elasticsearch/src/lib/client.js:37
    throw new Error('Do not reuse objects to configure the elasticsearch Client class: ' +
    ^

Error: Do not reuse objects to configure the elasticsearch Client class: https://github.com/elasticsearch/elasticsearch-js/issues/33
    at new Client (/root/geonames/node_modules/elasticsearch/src/lib/client.js:37:11)
    at module.exports (/root/geonames/node_modules/pelias-dbclient/src/client.js:13:17)
    at streamFactory (/root/geonames/node_modules/pelias-dbclient/src/sink.js:7:56)
    at module.exports (/root/geonames/lib/tasks/import.js:12:28)
    at Object.<anonymous> (/root/geonames/import.js:18:1)

Related issue: elastic/elasticsearch-js#33
I am also getting this error with the other main data importer sources, thus I opened the issue here.

Is this a configuration mistake on my end, or does the dbclient elastic connection need to be modified?

node 6.12
npm 3.10.10

@orangejulius
Copy link
Member

Hey @stevelacy,
This looks like a configuration issue. I vaguely remember that elasticsearch issue but don't know what caused it. If you post your pelias.json we can probably help find the issue.

@stephenlacy
Copy link
Author

I did a complete reinstall and did not see this issue again, thanks

@thucnc
Copy link

thucnc commented Apr 4, 2018

I got the same error when trying to build an api based on the importStream as below

var streamify = require('stream-array');
var streams = require('./stream/importPipeline');

app.post("/v1/pelias/import", (req, res) => {
    if (!req.body.address || !req.body.lat || !req.body.lng) return res.status(401).send({title: 'Missing required parameters: addres/lat/lng'})
    var address = req.body,
            stream  =  streams.importPipe(streamify([address]));

    stream.on('finish', (data) => {
        console.log(address || 'done')
        res.send({})
    })
})

The importPipeline (streams) run well on standalone.
@stevelacy , @orangejulius may you advise ?

@missinglink
Copy link
Member

missinglink commented Apr 4, 2018

hi @thucnc could you please post the code (or link to) './stream/importPipeline', I don't find it in this repo?

The error message Do not reuse objects to configure the elasticsearch Client class is coming from the elasticsearch client library and not from our code.

I can see in https://github.com/pelias/dbclient/blob/master/src/client.js that we use the singleton pattern to ensure that only one client connection is created.

We use the method settings.export() to ensure that the pelias config is copied, that way if the elasticsearch library mutates it (as mentioned in their link), then it will not affect the original configuration object.

Is it possible that you are closing the client connection and then trying to reuse the client?

@thucnc
Copy link

thucnc commented Apr 4, 2018

@missinglink My importPipeline code is as below:

streams.importPipe = function (stream) {
	streams.elasticsearch = require('pelias-dbclient');
	return stream
		 .pipe( streams.addressNormalizerAfterDedup ())
     .pipe( streams.adminLookup() )
     .pipe( streams.dbMapper() )
     .pipe( streams.elasticsearch({ batchSize: 1}) );
}

And actually, I workaround this issue by commenting out 3 lines of code in index.js of dbclient

//if (process.env.NODE_ENV !== 'test') {
//  require('./src/configValidation').validate(require('pelias-config').generate());
//}

However, at this point, I got another error, reporting here #17

@missinglink
Copy link
Member

missinglink commented Apr 4, 2018

as I suspected, the client connection is being closed after the first pipeline is ended in https://github.com/pelias/dbclient/blob/master/src/BatchManager.js#L121

and since it's a singleton, subsequent calls to ./client.js will return a reference to the closed client.

I guess we never intended the client to be used like this, although it seems like a fair thing to do.

one possible solution you could try is to edit https://github.com/pelias/dbclient/blob/master/src/client.js

module.exports = function(){
  return new elasticsearch.Client( settings.export().esclient || {} );
};

this will remove the singleton functionality and return a new client on every call to ./client.js

[edit] or even easier is to comment out this line: https://github.com/pelias/dbclient/blob/master/src/BatchManager.js#L121

let me know if that resolves your issue and I'll discuss with @orangejulius about getting it merged.

@missinglink
Copy link
Member

I'm going to re-open this issue as a feature request to allow the ./client.js to be reused by subsequent streams.

@missinglink missinglink reopened this Apr 4, 2018
@thucnc
Copy link

thucnc commented Apr 6, 2018

Hey @stevelacy, both this issue and #17 are solved with your suggestion:

module.exports = function(){
  return new elasticsearch.Client( settings.export().esclient || {} );
};

So, please apply this change on the next release. Really appreciate.

orangejulius added a commit that referenced this issue May 12, 2019
The singleton pattern has fallen out of favor lately, as it reduces the
flexibility of a module, and sometimes makes it harder to unit test.

Fixes #77
orangejulius added a commit that referenced this issue May 12, 2019
The singleton pattern has fallen out of favor lately, as it reduces the
flexibility of a module, and sometimes makes it harder to unit test.

More details on the singleton pattern: https://stackoverflow.com/questions/12755539/why-is-singleton-considered-an-anti-pattern

This change removes dbclient's use of the singleton pattern, which
should help folk who are using the module in ways we didn't anticipate.

Fixes #77
orangejulius added a commit that referenced this issue May 13, 2019
The singleton pattern has fallen out of favor lately, as it reduces the
flexibility of a module, and sometimes makes it harder to unit test.

More details on the singleton pattern: https://stackoverflow.com/questions/12755539/why-is-singleton-considered-an-anti-pattern

This change removes dbclient's use of the singleton pattern, which
should help folk who are using the module in ways we didn't anticipate.

Fixes #77
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 a pull request may close this issue.

4 participants