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

Remove stale 'namespace' config for JDBC lookups from doc #10886

Merged
merged 5 commits into from
Mar 5, 2021

Conversation

jihoonson
Copy link
Contributor

Description

The namespace config was removed in #2926. Also fixed a wrong instruction for the JDBC connector location.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@@ -268,22 +268,6 @@ export const LOOKUP_FIELDS: Field<LookupSpec>[] = [
},

// JDBC stuff
{
Copy link
Contributor

Choose a reason for hiding this comment

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

From looking at Travis failure, It looks like this requires a snapshot update. jest --updateSnapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I reverted this change in favor of #10888.

Copy link
Contributor

@capistrant capistrant left a comment

Choose a reason for hiding this comment

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

LGTM outside of the spellchecker flag by CI

@@ -377,7 +375,8 @@ The JDBC lookups will poll a database to populate its local cache. If the `tsCol
> If using JDBC, you will need to add your database's client JAR files to the extension's directory.
> For Postgres, the connector JAR is already included.
> For MySQL, you can get it from https://dev.mysql.com/downloads/connector/j/.
> Copy or symlink the downloaded file to `extensions/druid-lookups-cached-global` under the distribution root directory.
> The connector JAR should locate in the classpath of Druid's main class loader.
> An easy way to add it in the classpath is copying or symlinking the downloaded file to `lib/` under the distribution root directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like "symlinking" is called out by spell checker. I think it would be fine to add it to the dictionary in the website module. Might not be in a common dictionary, but from a computing standpoint it is a valid word IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it's valid, but deleted it by addressing @techdocsmith's comment below.

@jihoonson jihoonson changed the title Remove stale 'namespace' config for JDBC lookups from doc and web-console Remove stale 'namespace' config for JDBC lookups from doc Feb 18, 2021
@@ -377,7 +375,8 @@ The JDBC lookups will poll a database to populate its local cache. If the `tsCol
> If using JDBC, you will need to add your database's client JAR files to the extension's directory.
> For Postgres, the connector JAR is already included.
> For MySQL, you can get it from https://dev.mysql.com/downloads/connector/j/.
> Copy or symlink the downloaded file to `extensions/druid-lookups-cached-global` under the distribution root directory.
> The connector JAR should locate in the classpath of Druid's main class loader.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> The connector JAR should locate in the classpath of Druid's main class loader.
> The connector JAR should reside in the classpath of Druid's main class loader.

reside or "be located?"

@@ -377,7 +375,8 @@ The JDBC lookups will poll a database to populate its local cache. If the `tsCol
> If using JDBC, you will need to add your database's client JAR files to the extension's directory.
> For Postgres, the connector JAR is already included.
> For MySQL, you can get it from https://dev.mysql.com/downloads/connector/j/.
> Copy or symlink the downloaded file to `extensions/druid-lookups-cached-global` under the distribution root directory.
> The connector JAR should locate in the classpath of Druid's main class loader.
> An easy way to add it in the classpath is copying or symlinking the downloaded file to `lib/` under the distribution root directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> An easy way to add it in the classpath is copying or symlinking the downloaded file to `lib/` under the distribution root directory.
> To add the connector JAR to the classpath, you can copy the downloaded file to `lib/` under the distribution root directory. Alternatively, create a symbolic link to the connector in the `lib` directory.

@jihoonson
Copy link
Contributor Author

@techdocsmith thanks, I addressed your comments.

@jihoonson
Copy link
Contributor Author

@capistrant do you have more comments?

Copy link
Contributor

@capistrant capistrant left a comment

Choose a reason for hiding this comment

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

LGTM

@jihoonson
Copy link
Contributor Author

@capistrant thanks for the review.

@jihoonson jihoonson merged commit 16acd66 into apache:master Mar 5, 2021
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants