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

GH-40952: [Java][FlightSQL] Clean up flight-sql-jdbc-driver dependencies #40953

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

laurentgo
Copy link
Collaborator

@laurentgo laurentgo commented Apr 2, 2024

Rationale for this change

Module flight-sql-jdbc-driver declares multiple dependencies, some (joda) are not used anymore (but still packaged).

What changes are included in this PR?

Clean up list of dependencies declared in flight-sql-jdbc-driver modules as all of them are transitive dependencies from flight-sql-jdbc-core.

Are these changes tested?

Yes: build + existing shading test

Are there any user-facing changes?

No

…endencies

Module `flight-sql-jdbc-driver` declares multiple dependencies, some
(joda and bytebuddy) are not used anymore (but still packaged).

Clean up list of dependencies declared in flight-sql-jdbc-driver modules
as all of them are transitive dependencies from flight-sql-jdbc-core.
@laurentgo laurentgo requested a review from lidavidm as a code owner April 2, 2024 15:35
Copy link

github-actions bot commented Apr 2, 2024

⚠️ GitHub issue #40952 has been automatically assigned in GitHub to PR creator.

@laurentgo
Copy link
Collaborator Author

I thought bytebuddy was declared but not used, but it seems I got fooled by running dependency:tree and forgot that flight-sql-jdbc-core uses shading too and has a reduced pom.xml generated which causes issues when executing modules independently. Will address shortly

@laurentgo
Copy link
Collaborator Author

I'm noticing some build issues with archery:


Run archery docker run \
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.8.18/x64/bin/archery", line 8, in <module>
    sys.exit(archery())
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/click/core.py", line 1685, in invoke
    super().invoke(ctx)
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/runner/work/arrow/arrow/dev/archery/archery/docker/cli.py", line 67, in docker
    compose = DockerCompose(config_path, params=os.environ,
  File "/home/runner/work/arrow/arrow/dev/archery/archery/docker/core.py", line 169, in __init__
    self.config = ComposeConfig(config_path, dotenv_path, compose_bin,
  File "/home/runner/work/arrow/arrow/dev/archery/archery/docker/core.py", line 68, in __init__
    self._read_config(config_path, compose_bin)
  File "/home/runner/work/arrow/arrow/dev/archery/archery/docker/core.py", line 127, in _read_config
    result = compose.run(*args, env=self.env, check=False,
  File "/home/runner/work/arrow/arrow/dev/archery/archery/utils/command.py", line 78, in run
    return subprocess.run(invocation, **kwargs)
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/subprocess.py", line 493, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/subprocess.py", line 858, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/subprocess.py", line 1720, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'docker-compose'
Error: Process completed with exit code 1.

Is there anything I can do to address?

@lidavidm
Copy link
Member

lidavidm commented Apr 2, 2024

Install docker/docker-compose, for archery

Do we need to shade in -core? I had thought the point of having the split was not to shade

@laurentgo
Copy link
Collaborator Author

Install docker/docker-compose, for archery
The issue is actually for the Java/AMD64 JDK8 Ubuntu check

Do we need to shade in -core? I had thought the point of having the split was not to shade
I have no idea to be honest, I see a lot of shading in flight-sql-jdbc-core and vector but not sure if those shaded artifacts are actually used or not, and for what purpose. I would argue that maven-shade-modules should only be used in leaf modules as having dependencies between modules in the same reactor may cause weird behavior (the main one being that the classes and pom.xml being may change based on the maven invocation)

@lidavidm
Copy link
Member

lidavidm commented Apr 2, 2024

Ah, sorry.

@assignUser or @raulcd do we need to switch to docker compose instead of docker-compose for our actions now? actions/runner-images#9557 and the failing build is on a newer runner image than the passing builds

@assignUser
Copy link
Member

Yes that's right, Antoine is working on it here: #40949

@lidavidm
Copy link
Member

lidavidm commented Apr 2, 2024

Yes that's right, Antoine is working on it here: #40949

Ah whoops. We can ignore the failure here then. Thanks!

@lidavidm
Copy link
Member

lidavidm commented Apr 2, 2024

@github-actions crossbow submit java

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Apr 2, 2024
Copy link

github-actions bot commented Apr 2, 2024

Revision: 06b92a7

Submitted crossbow builds: ursacomputing/crossbow @ actions-1169653e46

Task Status
java-jars GitHub Actions
verify-rc-source-java-linux-almalinux-8-amd64 GitHub Actions
verify-rc-source-java-linux-conda-latest-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64 GitHub Actions
verify-rc-source-java-macos-amd64 GitHub Actions

@lidavidm lidavidm merged commit 41a989c into apache:main Apr 3, 2024
14 of 16 checks passed
@lidavidm lidavidm removed the awaiting merge Awaiting merge label Apr 3, 2024
Copy link

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 41a989c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

@laurentgo laurentgo deleted the laurentgo/dependencies-cleanup branch April 3, 2024 16:42
tolleybot pushed a commit to tmct/arrow that referenced this pull request May 2, 2024
…endencies (apache#40953)

### Rationale for this change

Module `flight-sql-jdbc-driver` declares multiple dependencies, some (joda) are not used anymore (but still packaged).

### What changes are included in this PR?

Clean up list of dependencies declared in flight-sql-jdbc-driver modules as all of them are transitive dependencies from flight-sql-jdbc-core.

### Are these changes tested?

Yes: build + existing shading test

### Are there any user-facing changes?

No
* GitHub Issue: apache#40952

Authored-by: Laurent Goujon <laurent@apache.org>
Signed-off-by: David Li <li.davidm96@gmail.com>
tolleybot pushed a commit to tmct/arrow that referenced this pull request May 4, 2024
…endencies (apache#40953)

### Rationale for this change

Module `flight-sql-jdbc-driver` declares multiple dependencies, some (joda) are not used anymore (but still packaged).

### What changes are included in this PR?

Clean up list of dependencies declared in flight-sql-jdbc-driver modules as all of them are transitive dependencies from flight-sql-jdbc-core.

### Are these changes tested?

Yes: build + existing shading test

### Are there any user-facing changes?

No
* GitHub Issue: apache#40952

Authored-by: Laurent Goujon <laurent@apache.org>
Signed-off-by: David Li <li.davidm96@gmail.com>
rok pushed a commit to tmct/arrow that referenced this pull request May 8, 2024
…endencies (apache#40953)

### Rationale for this change

Module `flight-sql-jdbc-driver` declares multiple dependencies, some (joda) are not used anymore (but still packaged).

### What changes are included in this PR?

Clean up list of dependencies declared in flight-sql-jdbc-driver modules as all of them are transitive dependencies from flight-sql-jdbc-core.

### Are these changes tested?

Yes: build + existing shading test

### Are there any user-facing changes?

No
* GitHub Issue: apache#40952

Authored-by: Laurent Goujon <laurent@apache.org>
Signed-off-by: David Li <li.davidm96@gmail.com>
rok pushed a commit to tmct/arrow that referenced this pull request May 8, 2024
…endencies (apache#40953)

### Rationale for this change

Module `flight-sql-jdbc-driver` declares multiple dependencies, some (joda) are not used anymore (but still packaged).

### What changes are included in this PR?

Clean up list of dependencies declared in flight-sql-jdbc-driver modules as all of them are transitive dependencies from flight-sql-jdbc-core.

### Are these changes tested?

Yes: build + existing shading test

### Are there any user-facing changes?

No
* GitHub Issue: apache#40952

Authored-by: Laurent Goujon <laurent@apache.org>
Signed-off-by: David Li <li.davidm96@gmail.com>
vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
…endencies (apache#40953)

### Rationale for this change

Module `flight-sql-jdbc-driver` declares multiple dependencies, some (joda) are not used anymore (but still packaged).

### What changes are included in this PR?

Clean up list of dependencies declared in flight-sql-jdbc-driver modules as all of them are transitive dependencies from flight-sql-jdbc-core.

### Are these changes tested?

Yes: build + existing shading test

### Are there any user-facing changes?

No
* GitHub Issue: apache#40952

Authored-by: Laurent Goujon <laurent@apache.org>
Signed-off-by: David Li <li.davidm96@gmail.com>
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.

3 participants