-
Notifications
You must be signed in to change notification settings - Fork 0
Incorporated SRV format in connection string #1
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
base: develop
Are you sure you want to change the base?
Conversation
pom.xml
Outdated
| <cdapArtifacts> | ||
| <parent>system:cdap-data-pipeline[6.8.0-SNAPSHOT,7.0.0-SNAPSHOT)</parent> | ||
| <parent>system:cdap-data-streams[6.8.0-SNAPSHOT,7.0.0-SNAPSHOT)</parent> | ||
| <parent>system:cdap-data-pipeline[6.11.0-SNAPSHOT,7.0.0-SNAPSHOT)</parent> |
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.
revert this change
pom.xml
Outdated
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <!-- version properties --> | ||
| <cdap.version>6.8.0-SNAPSHOT</cdap.version> | ||
| <cdap.version>6.11.0-SNAPSHOT</cdap.version> |
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.
revert this change
| @Name(MongoDBConstants.USE_SRV) | ||
| @Description("Enable SRV format for connection string") | ||
| @Nullable | ||
| private Boolean useSRV; |
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.
make it primitive at all places
| if (!containsMacro(MongoDBConstants.HOST) && Strings.isNullOrEmpty(host)) { | ||
| throw new InvalidConfigPropertyException("Host must be specified", MongoDBConstants.HOST); | ||
| } | ||
| if (!containsMacro(MongoDBConstants.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.
keep this condition as when useSRV is not macro and not true, this validation is required
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.
Done!
| private String password; | ||
|
|
||
| @Name(MongoDBConstants.USE_SRV) | ||
| @Description("Enable SRV format for connection string") |
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.
use Connect Using SRV String as field and label name in widget and also add the same in md doc file
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.
Add same description that you have added in md file
| @Name(MongoDBConstants.USE_SRV) | ||
| @Description("Enable SRV format for connection string") | ||
| @Nullable | ||
| private boolean useSRV; |
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.
change field name also
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.
Shall I make it as connectUsingSRV ?
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.
this name should be same as given in widget.json
| return password; | ||
| } | ||
|
|
||
| public boolean isUseSRV() { |
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.
adding 'is' as a prefix doesnt make much sense, keep it just connectUsingSRVString
|
|
||
| return connectionStringBuilder.toString(); | ||
|
|
||
|
|
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.
remove blank lines
| public static final String PASSWORD = "password"; | ||
|
|
||
| /** | ||
| * |
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.
Add a small description
| public MongoDBSinkConfig(String referenceName, String host, int port, String database, String collection, | ||
| String user, String password, String connectionArguments, String idField) { | ||
| super(referenceName, host, port, database, collection, user, password, connectionArguments); | ||
| String user, String password, Boolean useSRV, String connectionArguments, String idField) { |
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.
make boolean primitive at all places
|
|
||
| **Password:** Password to use to connect to the specified database. | ||
|
|
||
| **Connect Using SRV String:** Toggle to determine whether to use an SRV connection string for MongoDB. It can be |
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.
add same to sink also
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.
Done!
| /** | ||
| * Configuration property name used to specify whether to use an SRV Connection string for MongoDB. | ||
| */ | ||
| public static final String USE_SRV = "connectUsingSRVString"; |
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.
rename this also to CONNECT_USING_SRV_STRING
| @Description("Toggle to determine whether to use an SRV connection string for MongoDB. It can be " + | ||
| "enabled if the MongoDB deployment supports SRV DNS records for connection resolution.") | ||
| @Nullable | ||
| private boolean connectUsingSRVString; |
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.
I see this condition below to check this plugin property whether it contains macro or not, but the plugin property is not annotated with @macro annotation.
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.
Done!
| protected String collection; | ||
| protected String user; | ||
| protected String password; | ||
| protected Boolean connectUsingSRVString; |
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.
change to boolean ? as the main config file uses the primitive type for the same variable.
Boolean can cause NPE, if the variable is not set.
|
LGTM. |
Incorporated SRV format in connection string and introduced a toggle property in MongoDB plugin UI.