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

Allow parameters in CREATE/ALTER DATABASE TOPOLOGY #1911

Merged
merged 6 commits into from
Oct 31, 2024

Conversation

l-heemann
Copy link
Contributor

@l-heemann l-heemann commented Oct 25, 2024

@l-heemann
Copy link
Contributor Author

@HannesSandberg HannesSandberg self-assigned this Oct 25, 2024
@@ -36,6 +36,23 @@ However, over time, or after several `CREATE DATABASE` commands have been issued
At this point you can run `REALLOCATE DATABASES` to make the cluster re-balance databases across all servers that are part of the cluster.
====

The `CREATE DATABASE` and `ALTER DATABASE` commands support the usage of link:{neo4j-docs-base-uri}/cypher-manual/{page-version}/syntax/parameters[parameters] when providing the number of primaries and secondaries.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is overkill. We don't mention that the name could be a parameter. Maybe enough to add a second example to the create example. Something like:

CREATE DATABASE foo TOPOLOGY 3 PRIMARIES 2 SECONDARIES

or with parameters

{
  "dbname": "foo",
  "primary": 1,
  "secondary": 2
}

CREATE DATABASE $dbName TOPOLOGY $primary PRIMARIES $secondary SECONDARIES

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's useful to have both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the dbname 👍

I feel like it would be overkill to have one parameter example for create and another for alter alter, by mentioning ALTER in the text we get away with only the one. Let me know if you'd prefer the or with parameters approach with one for create and one for alter 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better practice is to keep them separate because whoever is looking for how to alter a database won't be looking at the create a database section. Of course, they might find a way to the example, but nobody reads the docs from top to bottom. A workaround could be to mention the feature, from when it is supported, etc., in the alter section and then refer to the create section, but it might be even more complicated.

Copy link
Contributor

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

I added some suggestions. Thanks, @l-heemann

@renetapopova
Copy link
Contributor

renetapopova commented Oct 25, 2024

Also, which version is this PR applicable to? I guess it's 5.26.

@l-heemann
Copy link
Contributor Author

I guess it's 5.26

Yes 👍

@@ -36,6 +36,24 @@ However, over time, or after several `CREATE DATABASE` commands have been issued
At this point you can run `REALLOCATE DATABASES` to make the cluster re-balance databases across all servers that are part of the cluster.
====

The `CREATE DATABASE` and `ALTER DATABASE` commands support the usage of link:{neo4j-docs-base-uri}/cypher-manual/{page-version}/syntax/parameters[parameters] when providing the database, the number of primaries and/or the number of secondaries.
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 `CREATE DATABASE` and `ALTER DATABASE` commands support the usage of link:{neo4j-docs-base-uri}/cypher-manual/{page-version}/syntax/parameters[parameters] when providing the database, the number of primaries and/or the number of secondaries.
The `CREATE DATABASE` and `ALTER DATABASE` commands support the usage of link:{neo4j-docs-base-uri}/cypher-manual/{page-version}/syntax/parameters[parameters] when providing the database, the number of primaries, and/or the number of secondaries.

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing Oxford comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer applicable, I think

@@ -36,6 +36,23 @@ However, over time, or after several `CREATE DATABASE` commands have been issued
At this point you can run `REALLOCATE DATABASES` to make the cluster re-balance databases across all servers that are part of the cluster.
====

The `CREATE DATABASE` and `ALTER DATABASE` commands support the usage of link:{neo4j-docs-base-uri}/cypher-manual/{page-version}/syntax/parameters[parameters] when providing the number of primaries and secondaries.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better practice is to keep them separate because whoever is looking for how to alter a database won't be looking at the create a database section. Of course, they might find a way to the example, but nobody reads the docs from top to bottom. A workaround could be to mention the feature, from when it is supported, etc., in the alter section and then refer to the create section, but it might be even more complicated.

@l-heemann
Copy link
Contributor Author

Corresponding changelog in docs-cypher: neo4j/docs-cypher#1082

@l-heemann
Copy link
Contributor Author

I think a better practice is to keep them separate because whoever is looking for how to alter a database won't be looking at the create a database section. Of course, they might find a way to the example, but nobody reads the docs from top to bottom. A workaround could be to mention the feature, from when it is supported, etc., in the alter section and then refer to the create section, but it might be even more complicated.

Makes sense, I've split it up similar to Hannes' first suggestion, let me know what you think.

Copy link
Contributor

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

The examples look good. I added a suggestion for the sentence before them.

modules/ROOT/pages/clustering/databases.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/databases.adoc Outdated Show resolved Hide resolved
@neo-technology-commit-status-publisher
Copy link
Collaborator

Thanks for the documentation updates.

The preview documentation has now been torn down - reopening this PR will republish it.

Copy link
Contributor

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

Thanks, @l-heemann. I think it looks good now.

Copy link
Contributor

@HannesSandberg HannesSandberg left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@renetapopova renetapopova merged commit 79daab1 into neo4j:dev Oct 31, 2024
8 checks passed
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