Skip to content

Docs: Point links in metrics-reporting.md to GitHub Java source#10397

Merged
nastra merged 2 commits into
apache:mainfrom
manuzhang:fix-javadoc
Jun 4, 2024
Merged

Docs: Point links in metrics-reporting.md to GitHub Java source#10397
nastra merged 2 commits into
apache:mainfrom
manuzhang:fix-javadoc

Conversation

@manuzhang
Copy link
Copy Markdown
Member

@manuzhang manuzhang commented May 29, 2024

It looks from the doc that RESTMetricsReporter is available for users. However, it's not a public class. This PR makes it public and add some javadocs. It also fixes its javadoc missing issue.

@manuzhang
Copy link
Copy Markdown
Member Author

manuzhang commented May 30, 2024

cc @nastra @jbonofre @Fokko

@Fokko
Copy link
Copy Markdown
Contributor

Fokko commented May 30, 2024

@manuzhang How I read the doc is:

  • You either enable the RESTMetricsReporter through the property, no need to override the class.
  • You implement a custom reporter by overriding the MetricsReporter, which is public.

@Fokko
Copy link
Copy Markdown
Contributor

Fokko commented May 30, 2024

@manuzhang I see your comment in #10378. I don't think that the class being private is the underlying issue of the missing javadoc. For example, the RestCatalog doesn't have docs either: https://iceberg.apache.org/docs/javadoc/1.5.2/org/apache/iceberg/rest/RESTCatalog.html

@Fokko
Copy link
Copy Markdown
Contributor

Fokko commented May 30, 2024

@manuzhang
Copy link
Copy Markdown
Member Author

RESTCatalog has javadoc at https://github.com/apache/iceberg/blob/javadoc/1.5.2/org/apache/iceberg/rest/RESTCatalog.html. It looks another issue when deploying to website.

@manuzhang
Copy link
Copy Markdown
Member Author

Default level of generating javadoc is public and protected. RESTMetricsReporter is package private so its javadoc is not generated.

@manuzhang
Copy link
Copy Markdown
Member Author

If we don't intend to make RESTMetricsReporter public, then we can just remove the javadoc link on "Metrics Reporting" page.

@Fokko
Copy link
Copy Markdown
Contributor

Fokko commented May 30, 2024

@manuzhang I think that would make more sense, but I would love to get @nastra input here (he's currently on holiday).

@Fokko
Copy link
Copy Markdown
Contributor

Fokko commented May 30, 2024

The RESTCatalog.html is there on the asf-site branch: https://github.com/apache/iceberg/blob/asf-site/javadoc/1.5.2/org/apache/iceberg/rest/RESTCatalog.html

@Fokko
Copy link
Copy Markdown
Contributor

Fokko commented May 30, 2024

The correct url is: https://iceberg.apache.org/javadoc/1.5.2/org/apache/iceberg/rest/RESTCatalog.html. Looks like the paths are not correct (/docs/ is in there).

@Fokko Fokko added this to the Iceberg 1.6.0 milestone May 30, 2024
@Fokko Fokko requested a review from nastra May 30, 2024 08:40
@Fokko
Copy link
Copy Markdown
Contributor

Fokko commented May 30, 2024

The URL in the metrics-reporting.md does look similar to the URLs that are working, so probably it is failing because the class is not in the Javadoc. Thanks again for reporting this @manuzhang 👍

Comment thread core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java Outdated
@github-actions github-actions Bot added the docs label May 30, 2024
@manuzhang manuzhang changed the title Core: Make RESTMetricsReporter public Docs: Point links in metrics-reporting.md to GitHub Java source May 30, 2024
Fokko added a commit to Fokko/iceberg that referenced this pull request May 30, 2024
Running the links checker from the CI turns out the be
very flaky: apache#10397

Let's remove it for now:

cc @manuzhang @pvary
Copy link
Copy Markdown
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

LGTM once CI passes

Comment thread core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java Outdated
….java

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>
Fokko added a commit to Fokko/iceberg that referenced this pull request Jun 3, 2024
Running the links checker from the CI turns out the be
very flaky: apache#10397

Let's remove it for now:

cc @manuzhang @pvary
@nastra nastra merged commit 0a26f02 into apache:main Jun 4, 2024
@manuzhang manuzhang deleted the fix-javadoc branch June 4, 2024 11:02
Fokko added a commit that referenced this pull request Jun 9, 2024
Running the links checker from the CI turns out the be
very flaky: #10397

Let's remove it for now:

cc @manuzhang @pvary
jasonf20 pushed a commit to jasonf20/iceberg that referenced this pull request Aug 4, 2024
sasankpagolu pushed a commit to sasankpagolu/iceberg that referenced this pull request Oct 27, 2024
sasankpagolu pushed a commit to sasankpagolu/iceberg that referenced this pull request Oct 27, 2024
Running the links checker from the CI turns out the be
very flaky: apache#10397

Let's remove it for now:

cc @manuzhang @pvary
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
Running the links checker from the CI turns out the be
very flaky: apache#10397

Let's remove it for now:

cc @manuzhang @pvary
jasonf20 pushed a commit to jasonf20/iceberg that referenced this pull request Sep 8, 2025
Running the links checker from the CI turns out the be
very flaky: apache#10397

Let's remove it for now:

cc @manuzhang @pvary
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