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

DOCSP-39685: Indexes landing page #75

Merged
merged 9 commits into from
Sep 10, 2024

Conversation

mcmorisi
Copy link
Collaborator

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-39685
Staging - https://preview-mongodbmcmorisi.gatsbyjs.io/java-rs/DOCSP-39685-indexes/indexes/

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Are all the links working?
  • Are the facets and meta keywords accurate?

Copy link

netlify bot commented Aug 19, 2024

Deploy Preview for docs-java-rs ready!

Name Link
🔨 Latest commit 4a07841
🔍 Latest deploy log https://app.netlify.com/sites/docs-java-rs/deploys/66e048e676243c00085ce397
😎 Deploy Preview https://deploy-preview-75--docs-java-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@norareidy norareidy left a comment

Choose a reason for hiding this comment

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

lgtm + some small things!

source/includes/usage-examples/index-code-examples.java Outdated Show resolved Hide resolved
source/indexes.txt Outdated Show resolved Hide resolved
source/indexes.txt Outdated Show resolved Hide resolved
@mcmorisi mcmorisi requested review from a team and stIncMale and removed request for a team August 20, 2024 15:02

collection.createIndex(Indexes.ascending("stars", "name"))
.subscribe(new PrintToStringSubscriber<String>());
.. include:: /includes/usage-examples/sample-app-intro.rst
Copy link
Member

Choose a reason for hiding this comment

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

Step 4 from this included file says

Copy the following code and paste it into a new Java file named WriteOperations.java.

WriteOperations.java is wrong for this PR, given that the class name in index-code-examples.java is IndexOperations .

MongoCollection<Document> collection = database.getCollection("movies");

// start-single-field
collection.createIndex(Indexes.ascending("<field name>"));
Copy link
Member

Choose a reason for hiding this comment

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

Similarly to other pages for the reactive driver, like #69, this one should both have the same note about Project Reactor Library, and actually execute the operations is demonstrates. collection.createIndex(Indexes.ascending("<field name>")) simply creates an object of the Publisher<String> type. It does not create an index. One way to execute the operation is, for example:

Mono<String> result = collection.createIndex(Indexes.ascending("<field name>"));
Mono.from(result).block();

This comment applies to many other examples in this PR.

import com.mongodb.reactivestreams.client.*;
import org.bson.Document;
import org.bson.conversions.Bson;
import reactor.core.publisher.Flux;
Copy link
Member

Choose a reason for hiding this comment

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

The imports here must be the same as in index-code-examples.java, otherwise some code from index-code-examples.java may not compile when copied in IndexOperations.

CreateCollectionOptions createCollectionOptions= new CreateCollectionOptions()
.clusteredIndexOptions(clusteredIndexOptions);

MongoCollection<Document> collection = database.createCollection("<collection name>",
Copy link
Member

Choose a reason for hiding this comment

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

The collection variable is declared in sample-index-application.java on line 32, which means, the another collection variable cannot be declared here.

Suggested change
MongoCollection<Document> collection = database.createCollection("<collection name>",
MongoCollection<Document> clusteredCollection = database.createCollection("<collection name>",

Copy link
Collaborator Author

@mcmorisi mcmorisi Aug 27, 2024

Choose a reason for hiding this comment

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

I can make this change for clarity's sake.

With that said, the individual examples in this code are intended to be separately copy-pasted by readers into the sample application provided at the start of the page (as opposed to running the entire sample-index-application.java file on its own). There may be some variable reuse in implementing your other feedback, FYI.

Copy link
Member

Choose a reason for hiding this comment

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

It's not for clarity. If you actually try to copy this example into the sample application, you'll discover that it does not compile for the reason I specified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I get it now! Appreciate the callouts!!

CreateCollectionOptions createCollectionOptions= new CreateCollectionOptions()
.clusteredIndexOptions(clusteredIndexOptions);

MongoCollection<Document> collection = database.createCollection("<collection name>",
Copy link
Member

Choose a reason for hiding this comment

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

database.createCollection does not return MongoCollection<Document>, it returns Publisher<Void>. Could you please try each example, to make sure it compiles and runs successfully?

@mcmorisi mcmorisi requested a review from stIncMale August 27, 2024 20:19
@rozza rozza requested review from rozza and removed request for stIncMale September 10, 2024 08:22
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

One nit and one question

MongoCollection<Document> collection = database.getCollection("movies");

// start-single-field
Publisher<String> result = collection.createIndex(Indexes.ascending("<field name>"));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: result is a misleading variable name as nothing has happened, these methods return a Publisher and nothing happens until its subscribed to.

Recommend renaming all variables called result.

@@ -0,0 +1,11 @@
You can use the following sample application to test the code examples on this
Copy link
Member

Choose a reason for hiding this comment

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

This seems out of place, given other examples in the documentation don't provide such information. Generally, they seem to provide a block about the examples using Project reactor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can add the extra info about using Project reactor to be more in line with the Read/Write landing pages.

@mcmorisi mcmorisi requested a review from rozza September 10, 2024 13:27
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcmorisi mcmorisi merged commit 8b3d199 into mongodb:main Sep 10, 2024
5 checks passed
@mcmorisi mcmorisi deleted the DOCSP-39685-indexes branch September 10, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants