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

Task to generate Painless API's per context #41233

Merged
merged 30 commits into from Apr 23, 2019

Conversation

Projects
None yet
6 participants
@jdconrad
Copy link
Contributor

commented Apr 16, 2019

This adds a gradle task called generateContextDoc in the Painless module. The task will start a cluster, issue commands against the context rest api for Painless, and generate documentation for each API per context. Each context has a first page of classes sorted by package first and class name second, along with a page per package with each classes' constructors, methods, and fields. A link is generated for each constructor, method, and field to a JavaDoc page when possible.

The change itself is small, but the number of lines is large due to the auto-generated changes in documentation.

Follow up: 1) Internal documentation. 2) Appropriately link each context to its API docs. 3) Add static methods/bindings. 4) Add class inheritance hierarchy. 5) Links per class/package. 6) Improve method links for declaring classes. 7) Explore reduction of total classes using hierachy. 8) Open-ended exploration of more automation for documentation per context.

Concern: With all the new links, the docs build process has slowed down some. I really hope that I don't have to reduce the number because I think the layout is significantly improved for figuring out what's available.

jdconrad and others added some commits Apr 2, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 16, 2019



include::painless-api-reference-score-java-lang.asciidoc[]
include::painless-api-reference-score-java-math.asciidoc[]

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 16, 2019

Contributor

I can't find this file in the PR. Is github's UI eating it?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 17, 2019

Author Contributor

They're added now :). Forgot to add the new files here.

@@ -67,7 +67,8 @@ class PluginBuildPlugin extends BuildPlugin {

configurePublishing(project, extension)

if (project.plugins.hasPlugin(TestClustersPlugin.class) == false) {
if (project.plugins.hasPlugin(TestClustersPlugin.class) == false
|| project.testClusters.findByName('integTest') == null) {

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 16, 2019

Contributor

I can't really speak to the gradle changes. You'll probably want another reviewer for that....

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 17, 2019

Author Contributor

Understood. I'm confident @rjernst can take a look at those :)

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira Apr 17, 2019

Contributor

What's the reason for this specific change?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 18, 2019

Author Contributor

Removed as it's not needed. Good catch. Believe this was left behind when figuring out why the task wasn't working.

@@ -0,0 +1,10 @@
[[painless-api-reference]]

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 16, 2019

Contributor

Should this be painless-api-context-reference?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 18, 2019

Author Contributor

@nik9000 If i leave it as painless-api-reference (need to change the name of the asciidoc) will this negate the need for a redirect as saved browser links will go to the new page at the current url?

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 22, 2019

Contributor

It will. I'm fine with that. Should the file's name stay the same?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 22, 2019

Author Contributor

Changed it back. Good catch, ty!

private final String rtn;
private final int readOnly;
private final List<String> parameters;
public final String declaring;

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 16, 2019

Contributor

Its probably worth a comment about why these are public instead. It'd be nice if we didn't need it though!

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 17, 2019

Author Contributor

Would you prefer these be getters instead or comments here?

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 17, 2019

Contributor

Hmmmm. I think getters would be more "normal java" to be honest. Personally, I think public final is fine but I think we're trying to be more "normal java".... 🤷‍♂️

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

+246,126 −6,464 Ah hahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahahaha.

Wow.

@nik9000
Copy link
Contributor

left a comment

Ah! I see the nature of the combinatorial explosion we're dealing with here. Do you think it'd be kinder to make files with the extra APIs that each context add on top of the global context? That'd save a ton of bits, speed up the docs build, and make it super clear what part of each context is unique.

@rjernst
Copy link
Member

left a comment

This looks ok, but I think we should see how much work deduplication is. With this change every context gets its own copy of every whitelisted class/method, while the vast majority of these are exactly the same.

import java.util.SortedMap;
import java.util.TreeMap;

public final class ContextDocGenerator {

This comment has been minimized.

Copy link
@rjernst

rjernst Apr 17, 2019

Member

A minimal javadoc explaining what this tool does would be nice for future modifications and understanding by others

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 17, 2019

Author Contributor

Agreed. Will add.

@jdconrad jdconrad added the WIP label Apr 17, 2019

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@nik9000 and @rjernst Thanks for the initial reviews. Agree completely on consolidating the APIs where possible. Going to add a WIP to this review for now, and work on that for the initial check in because it's clearly a bit of an increase in lines ;)

@@ -67,7 +67,8 @@ class PluginBuildPlugin extends BuildPlugin {

configurePublishing(project, extension)

if (project.plugins.hasPlugin(TestClustersPlugin.class) == false) {
if (project.plugins.hasPlugin(TestClustersPlugin.class) == false
|| project.testClusters.findByName('integTest') == null) {

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira Apr 17, 2019

Contributor

What's the reason for this specific change?

docCompile project(':modules:lang-painless')
}

ClusterConfiguration clusterConfig = project.extensions.create("generateContextCluster", ClusterConfiguration.class, project)

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira Apr 17, 2019

Contributor

@atorok we should probably avoid using ClusterFormationTasks and instead use the TestClustersPlugin here if that's possible. Would that work here?

This comment has been minimized.

Copy link
@rjernst

rjernst Apr 17, 2019

Member

We tried using testclusters, but it there are issues with it here. I don't think it is ready for general use yet.

This comment has been minimized.

Copy link
@atorok

atorok Apr 18, 2019

Contributor

We are using it for all plugins and modules now, so i'm +1 on adding new tests with testclusers. Wouldn't determinedly be interested in hearing why it didn't work.

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira Apr 18, 2019

Contributor

I agree with @atorok we are full swing migrating at this point. We should have very good reason for adding new usages of ClusterFormationTasks. The only valid reason I could see is if test clusters does not yet support a specific capability required by these tests.

This comment has been minimized.

Copy link
@rjernst

rjernst Apr 18, 2019

Member

IIRC, testclusters didn't work before because painless is a module, and even though we were not using it for integTest, the check in PluginBuildPlugin would throw an error since the testclusters plugin was applied in a module. Since it is now used by modules, we may be able to get it working.

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira Apr 19, 2019

Contributor

Yeah. I think that is supported now. @atorok can confirm.

This comment has been minimized.

Copy link
@rjernst

rjernst Apr 19, 2019

Member

Testclusters still does not work here. I think there needs to be more buildSrc tests for standalone uses, as I see a lot of initialization and wiring done inside PluginBuildPlugin. The configuration we tried was:

testClusters {
  docExtractor {
    version = project.version
    javaHome = project.file(project.ext.runtimeJavaHome)
    distribution = "DEFAULT"
  }
}

task generateContextDoc(type: JavaExec) {
  main = 'org.elasticsearch.painless.ContextDocGenerator'
  classpath = sourceSets.doc.runtimeClasspath
  useCluster testClusters.docExtractor
  systemProperty "cluster.uri", "${-> testClusters.docsExtractor.httpSocketURI}"
}

In addition to the extra settings of version and java home that should be defaulted to reasonable values and not require setting explicitly, it seems to fail to depend on actually building the distribution:

Caused by: org.elasticsearch.gradle.testclusters.TestClustersException: Can not start node{:modules:lang-painless:docExtractor-1}, missing: /mnt/ramdisk/elasticsearch/build/testclusters/extract/org.elasticsearch.distribution.elasticsearch/elasticsearch-8.0.0-SNAPSHOT

I don't think this PR should be blocked on making test clusters work for non integ tests.

Show resolved Hide resolved modules/lang-painless/build.gradle

sourceSets {
doc {
java {

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira Apr 17, 2019

Contributor

This section is redundant actually. The default convention is for source to live in src/<<source set name>>/java.

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 18, 2019

Author Contributor

Fixed.

jdconrad added some commits Apr 18, 2019

@jdconrad jdconrad force-pushed the jdconrad:docgen branch from 20aeb2d to a31d20c Apr 19, 2019

jdconrad added some commits Apr 21, 2019

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

@nik9000 and @rjernst Thanks for the reviews. I believe I have addressed all the feedback appropriately and this is ready for another round.

@mark-vieira Thanks for the review. Are you okay with the gradle task as it is for now based on the feedback from @rjernst ? As soon as the compatibility issues are handled, I'm happy to switch the task to use test clusters.

@mark-vieira

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@mark-vieira Thanks for the review. Are you okay with the gradle task as it is for now based on the feedback from @rjernst ?

That's fine. We can revisit this as part of the ongoing effort to migrate to test clusters.

=== Aggs API for package org.elasticsearch.xpack.sql.expression.function.scalar.whitelist
See the <<painless-api-reference-aggs, Aggs API>> for a high-level overview of all packages.

[[painless-api-reference-aggs-InternalSqlScriptUtils]]

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 22, 2019

Contributor

I think we should cut this out of the output. It isn't really "public".

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 22, 2019

Author Contributor

I agree with this sentiment. I don't have a great idea for how to do this and keep it automated. Any suggestions?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 22, 2019

Author Contributor

For now I've added a hardcoded list that can be added to easily in the meantime. In the future I would like to make this part of the entire whitelisting process where we can mark a class as internal and have it included in the PainlessLookup. Possibly, we can even ensure that a user can't execute the class members with an internal versus external compilation flag.

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 22, 2019

Contributor

👍


[[painless-api-reference-field-IntervalDayTime]]
==== IntervalDayTime
* boolean {java11-javadoc}/java.base/java/lang/Object.html#equals(java.lang.Object)[equals](Object)

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 22, 2019

Contributor

Should there be more stuff here?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 22, 2019

Author Contributor

This is a direct translation from the existing whitelist. I do want to make it more sophisticated so that it can recognize things that may not need to be included, but that I think that's probably a different PR.

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 22, 2019

Contributor

👍

@@ -0,0 +1 @@

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 22, 2019

Contributor

I wonder if we shouldn't emit empty files. It isn't a big deal but I figure it'd be nice.

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 22, 2019

Author Contributor

Agreed, and I've thought of this issue too. I would prefer to add this in a follow up PR as this one is already getting long.

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 22, 2019

Contributor

👍

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 22, 2019

Author Contributor

Decided to fit this one in and fix it.

@@ -0,0 +1,9 @@
[[painless-api-reference-interval]]

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 22, 2019

Contributor

I think we should have a note in any generated files. I havne't checked if this is generated though. If it isn't generated, I think it probably should be.

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 22, 2019

Author Contributor

It's generated. Will add a note.

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 22, 2019

Contributor

👍

@jdconrad jdconrad removed the WIP label Apr 22, 2019

@nik9000
Copy link
Contributor

left a comment

LGTM

I noticed in the "score" example it said The standard Painless API is available. and had a link to the root of the API page. This should probably mention that there is a special score API and maybe link directly to the standard API and the score API rather than the root, but I think that is a "for later" thing.

LGTM

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@nik9000 Thanks for the multiple rounds of reviews! You're right about the need for changes to the contexts documentation at this point. I'll follow up in another PR with changes for those.

@mark-vieira Are you okay with this code being committed as is? I didn't want to dismiss your review requesting changes without asking.

Just realized Mark is on vacation, so I'm going to dismiss this review as I believe I've covered all of his concerns.

@jdconrad jdconrad merged commit 9fc0e81 into elastic:master Apr 23, 2019

9 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/docs-check Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

jdconrad added a commit that referenced this pull request Apr 23, 2019

Task to generate Painless API's per context (#41233)
This adds a gradle task called generateContextDoc in the Painless module. The 
task will start a cluster, issue commands against the context rest api for 
Painless, and generate documentation for each API per context. Each context 
has a first page of classes sorted by package first and class name second, 
along with a page per package with each classes' constructors, methods, and 
fields. A link is generated for each constructor, method, and field to a JavaDoc 
page when possible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.