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

DOC-4549 improve RDI config reference #1086

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

andy-stark-redis
Copy link
Contributor

@andy-stark-redis andy-stark-redis commented Jan 22, 2025

DOC-4549 (but mainly refers to RDSC-2814)

I've reworked the content quite a bit to fit in with the docs style and also because the original formatting didn't work very well on the finished HTML page.

Aside from general review, a couple of things to note:

  • In the original document, database.encrypt had MySQL in the source database column, but referred to SQL Server in the description. I've assumed it should be SQL Server, but let me know if that is not correct.
  • At the end of the file, the targets.connection section includes some properties for SSL/TLS. Currently, the descriptions just say "Uncomment this line if you are using SSL/TLS". These lines contain the ${...} format to refer to the source secrets, so the user shouldn't modify them. However, do we want more description in this reference file, or should we just stick with the "uncomment this line" instruction?

@andy-stark-redis andy-stark-redis requested review from yaronp68 and a team January 22, 2025 11:32
Copy link
Collaborator

@dwdougherty dwdougherty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few things to look at. Approving.

| `database.encrypt` | `string` | SQL Server| If SSL is enabled for your SQL Server database, you should also enable SSL in RDI by setting the value of this property to `true`.<br/> Default: `false` |
| `database.server.id` | `integer` | MariaDB, MySQL | Numeric ID of this database client, which must be unique across all currently-running database processes in the MySQL cluster.<br/> Default: 1|
| `database.url` | `string` | Oracle | Specifies the raw database JDBC URL. Use this property to define a custom database connection. Valid values include raw TNS names and RAC connection strings.|
| `topic.prefix` | `string` | MariaDB, MySQL, Oracle, PostgreSQL, SQLServer | A prefix for all topic names that receive events emitted by this connector.<br/> Default: `"rdi"` |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy-stark-redis it's already defined in the advanced-source section. should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

| -- | -- | -- | -- |
| `host` | `string` | MariaDB, MySQL, Oracle, PostgreSQL, SQLServer| The IP address of the database instance. |
| `port` | `integer` | MariaDB, MySQL, Oracle, PostgreSQL, SQLServer | The port of the database instance. |
| `database` | `string` | Oracle, PostgreSQL, SQLServer| The name of the database to capture changes from. For `SQL Server` you can define this as comma-separated list of database names. |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra space between the SQL and Server

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


### Using custom queries in the initial snapshot {#custom-initial-query}

{{< note >}}This section is relevant only for MySQL/MariaDB, Oracle, PostgreSQL, and SQLServer.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy-stark-redis i think that MySQL/Mariadb should be replaced with MariaDB, MySQL.....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants