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.

Sign up for GitHub

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

Jump to bottom

Fix docs on website #7559

Merged
merged 4 commits into from
Sep 29, 2023
Merged

Fix docs on website #7559

merged 4 commits into from
Sep 29, 2023

Conversation

pratyakshsharma
Copy link
Contributor

Fixed docs at various places, that I came across as part of my PoC on Nessie.

dimas-b
dimas-b reviewed Sep 26, 2023
View reviewed changes
site/docs/develop/java.md Outdated
NessieApiV2 api = NessieClientBuilder.builder()
.withUri(URI.create("http://localhost:19121/api/v2"))
.build(NessieApiV2.class);
NessieApiV1 api = NessieClientBuilder.createClientBuilder(null, null)
Copy link
Member

Choose a reason for hiding this comment

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

API v2 is default now. V1 is deprecated. Does v2 not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v2 works, but line 27 above says NessieApiV1 should be used. If v2 is the default now, line 27 needs a change then.

Copy link
Member

Choose a reason for hiding this comment

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

Pls fix line 27 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dimas-b When you say API v2 is default now, I hope this covers all the scenarios, like flink with java, python etc. If so, docs might need to be corrected at few more places. For example - https://projectnessie.org/tools/iceberg/flink/#configuration.

Please confirm.

Copy link
Member

Choose a reason for hiding this comment

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

These examples are for direct java client interaction with Nessie Servers (e.g. custom tools). Engines like Spark, Flink, etc. interact with Nessie Servers via NessieCatalog, which is part of Iceberg jars. NessieCatalog has options for selecting the API version, but that is beyond the scope of this example. IIRC, we need to wait for Iceberg 1.4.0 to release before we can update examples for catalog properties to use v2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the explanation. This helps.

site/docs/develop/java.md Outdated Show resolved Hide resolved
site/docs/tools/iceberg/spark.md Show resolved Hide resolved
@snazy
Copy link
Member

snazy commented Sep 26, 2023

Thanks for the PR, @pratyakshsharma! Can you sign the CLA?

@pratyakshsharma
Copy link
Contributor Author

@snazy This is my second PR in nessie. I had signed the CLA in my first PR itself.

@snazy
Copy link
Member

snazy commented Sep 27, 2023

@snazy This is my second PR in nessie. I had signed the CLA in my first PR itself.

Ah, okay, nvm

snazy
snazy reviewed Sep 27, 2023
View reviewed changes
site/docs/develop/java.md Outdated Show resolved Hide resolved
site/docs/develop/java.md Outdated Show resolved Hide resolved
site/docs/develop/java.md Outdated Show resolved Hide resolved
dimas-b
dimas-b reviewed Sep 28, 2023
View reviewed changes
Copy link
Member

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

Generalli LGTM, just a minor comment, and please change the references example to stream() as discussed.

site/docs/develop/java.md Outdated Show resolved Hide resolved
@adutra adutra mentioned this pull request Sep 28, 2023
Merged
@pratyakshsharma pratyakshsharma mentioned this pull request Sep 28, 2023
Closed
@pratyakshsharma
Copy link
Contributor Author

Please take another pass @dimas-b

dimas-b
dimas-b previously approved these changes Sep 28, 2023
View reviewed changes
@dimas-b dimas-b requested a review from snazy September 28, 2023 21:54
snazy
snazy reviewed Sep 29, 2023
View reviewed changes
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

The changes look good to me. One thing I noticed is that the SQL example can't work without a CREATE NAMESPACE. Would you mind adding a CREATE NAMESPACE nessie.testing; to the SQL section?

@snazy snazy changed the title Fix docs Fix docs on website Sep 29, 2023
@pratyakshsharma
Copy link
Contributor Author

The changes look good to me. One thing I noticed is that the SQL example can't work without a CREATE NAMESPACE. Would you mind adding a CREATE NAMESPACE nessie.testing; to the SQL section?

Good point. I had faced this issue when I was trying out the commands. Let me add the instructions there.

@adutra
Copy link
Contributor

adutra commented Sep 29, 2023

A user on Zulip just spotted what seems to be another inconsistency:

https://projectnessie.org/tools/sql/#dropping-branchestags

There is no support for DROP IF EXISTS :-(

@pratyakshsharma do you want to include a fix for that as well?

@adutra
Copy link
Contributor

adutra commented Sep 29, 2023

See #7559.

@adutra
Copy link
Contributor

adutra commented Sep 29, 2023

There is no support for DROP IF EXISTS :-(

@pratyakshsharma do you want to include a fix for that as well?

Tackled in #7570, no need to change this PR!

dimas-b
dimas-b approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

Thanks a lot for you contribution @pratyakshsharma ! 👍

@dimas-b dimas-b merged commit a8c1a33 into projectnessie:main Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimas-b dimas-b dimas-b approved these changes

@snazy snazy snazy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pratyakshsharma @snazy @adutra @dimas-b