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

various MOLT corrections and fixes #19326

Merged
merged 9 commits into from
Feb 7, 2025
Merged

various MOLT corrections and fixes #19326

merged 9 commits into from
Feb 7, 2025

Conversation

taroface
Copy link
Contributor

@taroface taroface commented Jan 28, 2025

DOC-12144
DOC-12174

  • Fixed a regex example
  • Clarified cloud storage flags in examples
  • Removed some misleading links; clarified some wordings
  • Clarify required location of replicator binary

Copy link

github-actions bot commented Jan 28, 2025

Files changed:

Copy link

netlify bot commented Jan 28, 2025

Deploy Preview for cockroachdb-interactivetutorials-docs canceled.

Name Link
🔨 Latest commit ce5c5a5
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-interactivetutorials-docs/deploys/67a68e032c20c100080aa07e

Copy link

netlify bot commented Jan 28, 2025

Deploy Preview for cockroachdb-api-docs canceled.

Name Link
🔨 Latest commit ce5c5a5
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-api-docs/deploys/67a68e0415ea960008205ff7

Copy link

netlify bot commented Jan 28, 2025

Netlify Preview

Name Link
🔨 Latest commit ce5c5a5
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-docs/deploys/67a68e0348904d000885d524
😎 Deploy Preview https://deploy-preview-19326--cockroachdb-docs.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
Contributor

@ZhouXing19 ZhouXing19 left a comment

Choose a reason for hiding this comment

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

LGTM!

For previous binaries, refer to the [MOLT version manifest](https://molt.cockroachdb.com/molt/cli/versions.html). {% if page.name != "molt.md" %}For release details, see the [MOLT changelog]({% link releases/molt.md %}).{% endif %}
The following binaries are included:
- `molt`
- `replicator`. To use replication features, `replicator` **must** be located either in the same directory as `molt` or in a directory beneath `molt`.

Choose a reason for hiding this comment

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

@ryanluu12345 We should fix this since it was an issue that was reported recently where the directories beneath didnt seem to work


{% include_cached copy-clipboard.html %}
~~~
--bucket-path 's3://migration/data/cockroach'
--bucket-path 's3://migration/data/cockroach/?AWS_REGION=ap-south-1'

Choose a reason for hiding this comment

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

@ZhouXing19 @ryanluu12345 Is this the right format for this? I thought we don't respect anything with parsms for the bucket path. Just s3://some-name/path

Thats why the import-region flag was introduced.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's true.

We ignore the parameter passed via the --bucket-path.

So in this example we can do

--bucket-path 's3://migration/data/cockroach
--import-region 'ap-south-1'

and in the gcp example it can be

--bucket-path 'gs://migration/data/cockroach
--assume-role='[email protected]'
--use-implicit-auth

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'm very confused. The flag description says:

Required when AWS_REGION needs to be included in the --bucket-path URL. For example, if the s3 URL has AWS_REGION=ap-south-1, include --import-region=ap-south-1.

Also, for --bucket-path, we have:

Only the URL path is used; query parameters (e.g., credentials) are ignored. To include the AWS_REGION query parameter in an s3 URL, set the --import-region flag.

So basically "include the AWS_REGION in the s3 URL" doesn't mean literally including it in the s3 URL? It just means passing AWS_REGION to the IMPORT command internally? If so, then the current docs are pretty misleading IMO. @ZhouXing19 @Jeremyyang920

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the confusion!

So basically "include the AWS_REGION in the s3 URL" doesn't mean literally including it in the s3 URL?

Unfortunately yes. The AWS_REGION passed in the --bucket-path URL will be ignored.

Eventually, the real bucket path used for IMPORT will be this formula:

{bucket-path, stripped with all parameter}{/?AWS_REGION={value passed via --aws-region}}{&ASSUME_ROLE={value passed via --assume-role}}

cc @rohan-joshi @ryanluu12345 @Jeremyyang920 it seems that our current flag syntax is confusing.
For context, I was advocating having the user pass all these parameters via the bucket path, rather than using individual flags. I think this was discussed in one weekly that i wasn't able to make it, and eventually we settled with the individual flags solution. But given the usage seems confusing here, I wonder if it worths us reconsider the one-flag approach.

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 didn't mean to suggest that the flags are inherently confusing, just that I was still confused after all the discussion on how they work. I'd want to make the docs crystal-clear on this. Are the following true?

  • No query parameters should be used in any cloud storage URLs defined in --bucket-path
  • If you need to pass information that you would normally pass in a query parameter, use the flag instead (--import-region, --assume-role, --use-implicit-auth)

Copy link
Contributor

Choose a reason for hiding this comment

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

No query parameters should be used in any cloud storage URLs defined in --bucket-path

It's more that the parameters used in the bucket path will be ignored. It won't error out if being specified, but it won't take effect in the eventual url used by IMPORT.

If you need to pass information that you would normally pass in a query parameter, use the flag instead (--import-region, --assume-role, --use-implicit-auth)

Yes, only if those parameters are passed via flags, they will take effect in the eventual import url.

Copy link

@noelcrl noelcrl left a comment

Choose a reason for hiding this comment

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

Looks good so far! Not sure if this needs to be addressed now, but it seems that MOLT Verify flags for --table-filter and --schema-filter don't have their default values specified ('.*'), while their MOLT Fetch flags counterparts do.

@taroface taroface requested a review from ZhouXing19 February 7, 2025 15:31
@taroface
Copy link
Contributor Author

taroface commented Feb 7, 2025

Addressed your feedback @noelcrl @Jeremyyang920 @ZhouXing19, PTAL!

Copy link
Contributor

@ZhouXing19 ZhouXing19 left a comment

Choose a reason for hiding this comment

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

Thanks!!!!!

@taroface taroface requested a review from florence-crl February 7, 2025 15:41
Copy link
Contributor

@florence-crl florence-crl left a comment

Choose a reason for hiding this comment

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

lgtm pending suggestions

@taroface taroface enabled auto-merge (squash) February 7, 2025 21:50
@taroface taroface merged commit dac2007 into main Feb 7, 2025
6 checks passed
@taroface taroface deleted the migration-docs-fixit branch February 7, 2025 22:55
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.

5 participants