Skip to content

Conversation

@rymurr
Copy link
Contributor

@rymurr rymurr commented Oct 12, 2020

This change is Reviewable

@rymurr rymurr requested a review from a team October 12, 2020 11:53
@rymurr rymurr linked an issue Oct 12, 2020 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #301 into main will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #301      +/-   ##
============================================
- Coverage     54.02%   54.01%   -0.02%     
- Complexity        0        4       +4     
============================================
  Files           141      143       +2     
  Lines          4759     4771      +12     
  Branches        457      456       -1     
============================================
+ Hits           2571     2577       +6     
- Misses         2044     2050       +6     
  Partials        144      144              
Flag Coverage Δ Complexity Δ
#java 54.01% <ø> (-0.02%) 4.00 <ø> (+4.00) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...remio/nessie/server/providers/BackendProducer.java 50.00% <0.00%> (-5.56%) 0.00% <0.00%> (ø%)
...remio/nessie/backend/dynamodb/DynamoDbBackend.java 37.50% <0.00%> (-3.68%) 0.00% <0.00%> (ø%)
...ackend/dynamodb/AbstractEntityDynamoDbBackend.java 79.16% <0.00%> (-0.35%) 0.00% <0.00%> (ø%)
...nessie/backend/dynamodb/GitRefDynamoDbBackend.java 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
...sie/backend/dynamodb/GitObjectDynamoDbBackend.java 90.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
.../com/dremio/nessie/hms/Hive2PartitionFilterer.java 76.92% <0.00%> (ø) 4.00% <0.00%> (?%)
.../org/apache/hadoop/hive/metastore/api/Catalog.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0507d2e...e595527. Read the comment docs.

Copy link
Contributor

@laurentgo laurentgo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 15 files reviewed, 4 unresolved discussions (waiting on @rymurr)


.gitignore, line 54 at r2 (raw file):

# gradle

tools/apprunner-gradle-plugin/.gradle

is it better to only have one .gitignore or to create a new one under tools/apprunner-gradle-plugin?


.github/workflows/main.yml, line 76 at r2 (raw file):

          restore-keys: ${{ runner.os }}-gradle
      - name: Populate version from maven
        run: mvn -B prepare-package --file ../pom.xml -pl :nessie-tools

shouldn't the maven artifact used instead?


tools/pom.xml, line 48 at r2 (raw file):

              <path>apprunner-gradle-plugin/version.txt</path>
              <lines>
                <line>${project.version}</line>

is the goal simply to get the version in a file? I think you can simply do it with a maven command, without having to add a plugin:

./mvnw help:evaluate -Dexpression=project.version -q -DforceStdout

tools/apprunner-gradle-plugin/src/main/java/com/dremio/nessie/quarkus/gradle/QuarkusApp.java, line 223 at r2 (raw file):

    }
    rtProps = new Properties();
    try (BufferedReader reader = Files.newBufferedReader(path)) {

let's make sure to use the UTF8 encoding

Copy link
Contributor Author

@rymurr rymurr left a comment

Choose a reason for hiding this comment

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

@laurentgo any thoughts on how to get rid of the static variables in StopTask I can follow your pattern in the maven plugin but I don't know how to easily inform the test client without code duplication

Reviewable status: 0 of 15 files reviewed, 4 unresolved discussions (waiting on @laurentgo)


.gitignore, line 54 at r2 (raw file):

Previously, laurentgo (Laurent Goujon) wrote…

is it better to only have one .gitignore or to create a new one under tools/apprunner-gradle-plugin?

ive never seen multiple .gitignores in a project, not super keen on the idea. I have removed the tools/apprunner-gradle-plugin bit however.


.github/workflows/main.yml, line 76 at r2 (raw file):

Previously, laurentgo (Laurent Goujon) wrote…

shouldn't the maven artifact used instead?

since this is a different job the mvn artifact may not be available. I can instead run a cli script to pull the version out of the parent pom if you would rather do that?


tools/pom.xml, line 48 at r2 (raw file):

./mvnw help:evaluate -Dexpression=project.version -q -DforceStdout
ok, much easier. Have done that now


tools/apprunner-gradle-plugin/src/main/java/com/dremio/nessie/quarkus/gradle/QuarkusApp.java, line 223 at r2 (raw file):

Previously, laurentgo (Laurent Goujon) wrote…

let's make sure to use the UTF8 encoding

Done.

Copy link
Contributor

@laurentgo laurentgo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 14 files reviewed, 5 unresolved discussions (waiting on @laurentgo and @rymurr)


.github/workflows/main.yml, line 76 at r2 (raw file):

Previously, rymurr (Ryan Murray) wrote…

since this is a different job the mvn artifact may not be available. I can instead run a cli script to pull the version out of the parent pom if you would rather do that?

I didn't realized at first this was done to get the project version


tools/apprunner-gradle-plugin/src/main/java/com/dremio/nessie/quarkus/gradle/QuarkusApp.java, line 78 at r3 (raw file):

  }

  public static QuarkusApp newApplication(Configuration configuration, Project project) {

I updated my config patch to split the newApplication in two parts, one maven specific and one for quarkus. Would that be helpful for the gradle plugin?


tools/apprunner-gradle-plugin/src/main/java/com/dremio/nessie/quarkus/gradle/StartTask.java, line 47 at r3 (raw file):

      if (existingApplication()) {
        getLogger().info("Quarkus application already started, incrementing counter.");
        incrementCount();

tbh, I'm not sure I understand why this is necessary. Can gradle invoke multiple times the start task for the same execution?


tools/apprunner-maven-plugin/pom.xml, line 61 at r3 (raw file):

    <plugins>
      <plugin>
        <groupId>ru.yaal.maven</groupId>

to be removed I guess?

Copy link
Contributor Author

@rymurr rymurr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 14 files reviewed, 5 unresolved discussions (waiting on @laurentgo)


tools/apprunner-gradle-plugin/src/main/java/com/dremio/nessie/quarkus/gradle/QuarkusApp.java, line 78 at r3 (raw file):

Previously, laurentgo (Laurent Goujon) wrote…

I updated my config patch to split the newApplication in two parts, one maven specific and one for quarkus. Would that be helpful for the gradle plugin?

cool, will pick up once its merged


tools/apprunner-gradle-plugin/src/main/java/com/dremio/nessie/quarkus/gradle/StartTask.java, line 47 at r3 (raw file):

Previously, laurentgo (Laurent Goujon) wrote…

tbh, I'm not sure I understand why this is necessary. Can gradle invoke multiple times the start task for the same execution?

Yes. If we have 3 modules that need quarkus for tests then Gradle will run 3 quarkus tasks simultaneously at the start of the job. This allows one to win and the other to increment the usage count at the cost of blocking gradle until quarkus starts up and a static ref count. An easier approach would be to start quarkus once if needed and stop once if needed across the entire project. I haven't figured out how to express this in gradle tasks yet though.


tools/apprunner-maven-plugin/pom.xml, line 61 at r3 (raw file):

Previously, laurentgo (Laurent Goujon) wrote…

to be removed I guess?

Done.

Copy link
Contributor

@laurentgo laurentgo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 14 files reviewed, 3 unresolved discussions (waiting on @laurentgo and @rymurr)


tools/apprunner-gradle-plugin/src/main/java/com/dremio/nessie/quarkus/gradle/StartTask.java, line 47 at r3 (raw file):

Previously, rymurr (Ryan Murray) wrote…

Yes. If we have 3 modules that need quarkus for tests then Gradle will run 3 quarkus tasks simultaneously at the start of the job. This allows one to win and the other to increment the usage count at the cost of blocking gradle until quarkus starts up and a static ref count. An easier approach would be to start quarkus once if needed and stop once if needed across the entire project. I haven't figured out how to express this in gradle tasks yet though.

but I guess it's not the same execution but 3 different executions, isn't it? The compiler task would not only run once and block for the other modules until all modules are done?

Copy link
Contributor Author

@rymurr rymurr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 14 files reviewed, 3 unresolved discussions (waiting on @laurentgo)


tools/apprunner-gradle-plugin/src/main/java/com/dremio/nessie/quarkus/gradle/StartTask.java, line 47 at r3 (raw file):

Previously, laurentgo (Laurent Goujon) wrote…

but I guess it's not the same execution but 3 different executions, isn't it? The compiler task would not only run once and block for the other modules until all modules are done?

I am not 100% sure what you mean. Will try to answer: The 3 quarkus-start tasks will run simultaneously at the start of the gradle build. This would start 1 server and increment the ref count to 3. The first one would block the other two till its started. This blocks 2 threads in the gradle process but others are still making progress. Once all quarkus engines are 'started ' the build process continues as normal. The threaded, graph-based nature of gradle seems super powerful for certain tasks but it makes this type of task a bit annoying. Hope thats close to what you were asking?

@jacques-n
Copy link
Contributor

Hey @rymurr and @laurentgo, let's get this wrapped up so we can start getting reviews on apache/iceberg#1587

@rymurr rymurr force-pushed the gradle-plugin-attempt branch from 2a9811a to cba8625 Compare October 20, 2020 17:57
@jacques-n jacques-n requested a review from laurentgo October 21, 2020 22:41
laurentgo
laurentgo previously approved these changes Oct 21, 2020
Copy link
Contributor

@laurentgo laurentgo left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 15 files at r1, 1 of 6 files at r3, 3 of 4 files at r4, 1 of 10 files at r5, 2 of 2 files at r6.
Reviewable status: 8 of 18 files reviewed, 3 unresolved discussions (waiting on @laurentgo)


tools/apprunner-gradle-plugin/src/main/java/com/dremio/nessie/quarkus/gradle/QuarkusAppExtension.java, line 23 at r6 (raw file):

public class QuarkusAppExtension {
  private MapProperty<String, Object> props;

final?

@rymurr
Copy link
Contributor Author

rymurr commented Oct 22, 2020

fixed the 'final' but it dismissed the approval. Could I get another @laurentgo ? Or feel free to merge it :-)

Copy link
Contributor

@laurentgo laurentgo left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 15 files at r1, 8 of 10 files at r5, 1 of 1 files at r7.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @laurentgo)

@laurentgo laurentgo merged commit d3052ef into projectnessie:main Oct 22, 2020
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.

Gradle plugin for quarkus/nessie

3 participants