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

Move web-console dependency declaration from druid-server to druid-distribution #12501

Merged
merged 6 commits into from
Sep 15, 2022

Conversation

FrankChen021
Copy link
Member

@FrankChen021 FrankChen021 commented May 4, 2022

Description

This is a follow up of #12486

#12486 removes the web-console dependency from druid-server module because the dependency check reports that it's not used in druid-server module.

Because druid-server module is referenced by many other modules, the existence of web-console dependency in druid-server module makes that module to be compiled after the web-console module. However, building of web-console module usually takes a long time. If the whole module is built in parallel with -T mvn parameter, this dependency makes the building slower.

After #12486 is merged, @vogievetsky found that this dependency must be there or the web-console module won't be shipped in the distribution package(#12486 (comment))

The druid distribution package is packaged by the mvn-assembly-plugin referenced in the druid-distribution module.

druid/distribution/pom.xml

Lines 256 to 258 in 0206a2d

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-assembly-plugin</artifactId>

This plugin is also configured to copy all dependencies declared in the pom of druid-distribution to the lib directory as below.

<dependencySet>
<useProjectArtifact>false</useProjectArtifact>
<useTransitiveDependencies>true</useTransitiveDependencies>
<outputDirectory>lib</outputDirectory>

So, we can move the dependency declaration from the druid-server module to the druid-distribution module so that we still gain the building speed up as well as the package integrity.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@FrankChen021 FrankChen021 added the Area - Dev For items related to the project itself, like dev docs and checklists, but not CI label May 4, 2022
@paul-rogers
Copy link
Contributor

@FrankChen021, thanks for making this fix: the revised dependency looks right: it should be distribution that depends on the console, not some module which distribution depends on. That is, make the dependency direct, not transitive.

Adding an IT is usually a Good Thing. There is, however, a big cost to adding a test group in the current setup: we build the entire Druid stack for each test group. I've been chipping away at First cut at restructuring the integration tests to make the ITs simpler and faster: your new test is a good candidate to port.

In the mean while, can the IT be added to an existing group? It can just piggy-back on any one of the existing clusters, can't it?

@FrankChen021
Copy link
Member Author

@paul-rogers Thanks for the review.

I need to make some changes to the CI. Since the web-console is moved to distribution, building of IT module does not involve packaging the web-console module, the newly added test can not pass. To make the test in a separated group can let it run independently in a separated CI job, in that way, other jobs don't need to packaging the web-console module which can save time.

@FrankChen021
Copy link
Member Author

Since #12776 has moved web-console e2e test to phase 1, there's no need to an IT test anymore, revert these changes.

@FrankChen021
Copy link
Member Author

Hello @clintropolis @vogievetsky Could you review this PR?

@FrankChen021
Copy link
Member Author

By this adjustment, the building speed with -T 1C parameter on mvn command is improved from about 2:50min to about 2min on my Mac book.

@FrankChen021
Copy link
Member Author

Hello @clintropolis Any update on this PR?

Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

sorry for the delay, forgot about this one.
I confirmed that everything still works, so this seems fine to me 👍

@FrankChen021
Copy link
Member Author

Thank you @clintropolis

@FrankChen021 FrankChen021 merged commit aa9b090 into apache:master Sep 15, 2022
@FrankChen021 FrankChen021 deleted the console-dependency branch September 15, 2022 02:39
@clintropolis
Copy link
Member

hmm, I think this might have broken how I was debugging druid services in intellij using run configurations (similar to https://github.com/apache/druid/blob/master/dev/intellij-setup.md#coordinatorxml), since the web-console is no longer working for me. I haven't verified that it is this change yet, but it seems pretty likely given that the above setup is not using the distribution packaging at all.

@clintropolis
Copy link
Member

I haven't verified that it is this change yet, but it seems pretty likely given that the above setup is not using the distribution packaging at all.

confirmed it is related to this change, I'll see if I can find a solution to my debugger problem, since I use that method of debugging fairly frequently...

@FrankChen021
Copy link
Member Author

FrankChen021 commented Sep 20, 2022

Hello @clintropolis

Sorry to hear that. I didn't use debug from Intelli to launch process, instead I usually use 'attach' to an existing running process.

Since web-console is served by router, do you mean you debug router in this way?

@FrankChen021
Copy link
Member Author

I'm wondering if we can use 'profile' to include the web-console in the server/pom.xml and active such profile from intellij when debugging?

@FrankChen021
Copy link
Member Author

I'm wondering if we can use 'profile' to include the web-console in the server/pom.xml and active such profile from intellij when debugging?

I think it's possible. Intellij provides a profile panel that allows us to active specific profiles.

image

We can keep that newly added profile selected from Intellij. @clintropolis Do you have any other ideas?

@clintropolis
Copy link
Member

clintropolis commented Sep 20, 2022

I think I've got it working well enough using -Xbootclasspath to customize what the router has access to when it starts up.

Since web-console is served by router, do you mean you debug router in this way?

Yeah, I have a handful of xml files that I use to launch druid processes directly within intellij. Extensions (and now the web-console) are a bit funny in this model as you have to externally build them and refer to their location, but works pretty well otherwise.

Here is my Router.xml for example (after fixing):

<component name="ProjectRunConfigurationManager">
  <configuration default="false" name="Router" type="Application" factoryName="Application">
    <option name="ALTERNATIVE_JRE_PATH" value="1.8" />
    <option name="MAIN_CLASS_NAME" value="org.apache.druid.cli.Main" />
    <module name="druid-services" />
    <option name="PROGRAM_PARAMETERS" value="server router" />
    <option name="VM_PARAMETERS" value="-server -Xbootclasspath/a:$PROJECT_DIR$/conf/router/:$PROJECT_DIR$/web-console/target/web-console-25.0.0-SNAPSHOT.jar -Duser.timezone=UTC -Dfile.encoding=UTF-8 -Xmx256M -Xmx256M -XX:+UseG1GC -Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager -Dorg.jboss.logging.provider=slf4j -Dlog4j.configurationFile=$PROJECT_DIR$/core/src/main/resources/log4j2.debug.xml -Ddruid.host=localhost -Ddruid.zk.service.host=localhost -Ddruid.emitter=logging -Ddruid.router.managementProxy.enabled=true -Ddruid.extensions.directory=$PROJECT_DIR$/distribution/target/extensions/ -Ddruid.extensions.loadList=[\&quot;druid-histogram\&quot;,\&quot;druid-bloom-filter\&quot;,\&quot;druid-datasketches\&quot;,\&quot;druid-lookups-cached-global\&quot;,\&quot;druid-basic-security\&quot;] -Ddruid.server.http.numThreads=50  -Ddruid.generic.useDefaultValueForNull=false " />
  </configuration>
</component>

If you put these xml files in .idea/runConfigurations/ then you have all of these options to run various services in the run/debug dropdown menu:
Screen Shot 2022-09-19 at 7 13 42 PM

I do this because in my experience its got a faster turn-around than building a quickstart and attaching, can modify most stuff on the fly and live reload, etc.

@FrankChen021
Copy link
Member Author

Glad to hear it works. Sorry for the inconvenience caused by this change. @clintropolis

@kfaraz kfaraz added this to the 25.0 milestone Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area - Dev For items related to the project itself, like dev docs and checklists, but not CI Dev Productivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants