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
[ZEPPELIN-5991] Adding Support of DocumentDb and Mongo Atlas for MongoDB interpreter #4706
base: master
Are you sure you want to change the base?
Conversation
Add Support for AWS documentDB and Atlas
removing globalThis as it generate errors
adding doc
Thank you for your first contribution :-) I'll review this PR soon |
|
||
/* adding support for protocal like mongodb+srv for atlas cluster*/ | ||
String mongoProtocol = getProperty("mongo.server.protocol", "mongodb"); | ||
if ("mongodb".equalsIgnoreCase(mongoProtocol)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please check your indentation settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure will update indentation.
if ("mongodb".equalsIgnoreCase(mongoProtocol)){ | ||
dbAddress = getProperty("mongo.server.host") + ":" + getProperty("mongo.server.port"); | ||
}else{ | ||
dbAddress = mongoProtocol +"://"+ getProperty("mongo.server.host"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have a protocol value, does it mean mongo.server.port
? Otherwise, don't some protocols need port value? Could you please verify it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if protocol is mongodb
then it will use mongo.server.port
as many provider use different port for mongo / documentDb or similar service.
but i never seen mongodb+srv
protocol is been used with different port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the response. BTW, for the conservative way, could you please add the code for port availability? I also don't know if it's really needed but I think it would be better to add it anyway. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok will update the code as requested.
@@ -117,6 +125,67 @@ public InterpreterResult interpret(String script, InterpreterContext context) { | |||
executor.setWatchdog(new ExecuteWatchdog(commandTimeout)); | |||
|
|||
final CommandLine cmdLine = CommandLine.parse(getProperty("mongo.shell.path")); | |||
/* added support for API versions */ | |||
String apiVersion = getProperty("mongo.server.api.version", ""); | |||
if (!"".equalsIgnoreCase(apiVersion)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: StringUtils.isBlank(...)
?
|
||
/* adding support for SSL for and TLS for documentDB */ | ||
String runWithSSL = getProperty("mongo.server.ssl.enabled", "false"); | ||
if ("true".equalsIgnoreCase(runWithSSL)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Boolean.valueOf(...)
or Boolean.parseBoolean(...)
?
I left some comments. Could you please check it? Moreover, I think it would be good to have some tests for your contribution. |
What is this PR for?
Adding Support of DocumentDb and Mongo Atlas for MongoDB interpreter.
DocumentDB in AWS required SSL/ TSL connection which was not supported.
MongoDB Atlas required special protocal to connect
mongodb+srv
What type of PR is it?
Improvement
Todos
What is the Jira issue?
ZEPPELIN-5991
How should this be tested?
Questions: