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

[#1193] improvement(docs): Add the document about how to debug trino connector locally. #2446

Merged
merged 8 commits into from
Mar 14, 2024

Conversation

yuqi1129
Copy link
Contributor

@yuqi1129 yuqi1129 commented Mar 7, 2024

What changes were proposed in this pull request?

Add a document about how to debug develop and debug trino-connector locally.

Why are the changes needed?

To make users more knowledgeable about Gravitino trino connector.

Fix: #1193

Does this PR introduce any user-facing change?

N/A.

How was this patch tested?

N/A.

Comment on lines 146 to 162
<dependency>
<groupId>com.datastrato.gravitino</groupId>
<artifactId>gravitino-client-java-runtime</artifactId>
<version>0.4.0-SNAPSHOT</version>
</dependency>

<dependency>
<groupId>com.datastrato.gravitino</groupId>
<artifactId>bundled-catalog</artifactId>
<version>0.4.0-SNAPSHOT</version>
</dependency>

<dependency>
<groupId>com.datastrato.gravitino</groupId>
<artifactId>catalog-common</artifactId>
<version>0.4.0-SNAPSHOT</version>
</dependency>
Copy link
Contributor

@xiacongling xiacongling Mar 7, 2024

Choose a reason for hiding this comment

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

these packages are not in maven central repository. can you publish them? or can we build and publish them locally from source code?

Copy link
Contributor

Choose a reason for hiding this comment

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

shall we use current snapshots (0.5.0-SNAPSHOT)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe you can install them with the latest version 0.5.0-SNAPSHOT locally.

@yuqi1129
Copy link
Contributor Author

yuqi1129 commented Mar 8, 2024

@diqiu50 ,
Could you kindly review it for me?


<dependency>
<groupId>com.datastrato.gravitino</groupId>
<artifactId>gravitino-client-java-runtime</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no such artifact after a local publish. do you mean client-java-runtime?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest providing a script to install Graviton-related dependencies into the local Maven repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no such artifact after a local publish. do you mean client-java-runtime?

Exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest providing a script to install Graviton-related dependencies into the local Maven repository.

I will convert the artifacts to the standard release versions and the user can download them from Maven central repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the Maven Central Repository is a good choice. We need to consider situations where a user modifies a class in the client-java-runtime. In such cases, the user might not be able to install the new package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, a local repo is necessary.

docs/assets/trino/pom.xml Outdated Show resolved Hide resolved
Comment on lines +16 to +27
2. Create a catalog in the Gravitino server, for more information, please refer to the [Gravitino metadata management](../manage-metadata-using-gravitino.md). Assuming we have just created a MySQL catalog using the following command:

```curl
curl -X POST -H "Content-Type: application/json" -d '{"name":"test","comment":"comment","properties":{}}' http://localhost:8090/api/metalakes

curl -X POST -H "Content-Type: application/json" -d '{"name":"mysql_catalog3","type":"RELATIONAL","comment":"comment","provider":"jdbc-mysql", "properties":{
"jdbc-url": "jdbc:mysql://127.0.0.1:3306?useSSL=false&allowPublicKeyRetrieval=true",
"jdbc-user": "root",
"jdbc-password": "123456",
"jdbc-driver": "com.mysql.cj.jdbc.Driver"
}}' http://localhost:8090/api/metalakes/test/catalogs
```
Copy link
Contributor

Choose a reason for hiding this comment

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

catalogs are innecessary for setting up dev environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you use the Trino command line to display the Gravition catalog if you do not create catalogs here?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. i just start gravitino server built from source, and add a test empty metalake. on trino side, there is only gravitino.properties under the catalog dir, and Trino dev server started up normally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean if we could list the catalogs in the format like "test.mysql_catalog" by the Trino client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like
image

docs/trino-connector/development.md Outdated Show resolved Hide resolved
@xiacongling
Copy link
Contributor

Setting up with Trino 421 successfully. I've encountered some problems during the setup:

  1. recommanded to use Trino ≥426 (unfortunately, we are using Trino 421). in my case, ConnectorMetadata.dropSchema has only 2 arguments (cascade dropping was not supportted yet and this interface method was removed in trino 426), so i need to update gravitino's implementation, rebuild the project and publish artifacts to local maven repository;
  2. struggled with signing for some time when publishing my own artifactories to local repository, can we make signing an optional task and skip it by default? we only need to apply signing in public releases, right?

@yuqi1129
Copy link
Contributor Author

yuqi1129 commented Mar 9, 2024

@diqiu50

recommanded to use Trino ≥426 (unfortunately, we are using Trino 421). in my case, ConnectorMetadata.dropSchema has only 2 arguments (cascade dropping was not supportted yet and this interface method was removed in trino 426), so i need to update gravitino's implementation, rebuild the project and publish artifacts to local maven repository;

About this point, please take a look if we need to modify the supported versions, as we only test and verify versions newer than 426.

struggled with signing for some time when publishing my own artifactories to local repository, can we make signing an optional task and skip it by default? we only need to apply signing in public releases, right?

Yeah, it would be great help if we can skip the signing stage. currently, you can use maven to install to your local repo, or can you work on it?

@diqiu50
Copy link
Contributor

diqiu50 commented Mar 11, 2024

@diqiu50

recommanded to use Trino ≥426 (unfortunately, we are using Trino 421). in my case, ConnectorMetadata.dropSchema has only 2 arguments (cascade dropping was not supportted yet and this interface method was removed in trino 426), so i need to update gravitino's implementation, rebuild the project and publish artifacts to local maven repository;

About this point, please take a look if we need to modify the supported versions, as we only test and verify versions newer than 426.

struggled with signing for some time when publishing my own artifactories to local repository, can we make signing an optional task and skip it by default? we only need to apply signing in public releases, right?

Yeah, it would be great help if we can skip the signing stage. currently, you can use maven to install to your local repo, or can you work on it?

I have verified it; indeed, the dropSchema interface in Trino-421 does not include 'cascade'. We need to update the documentation.

I think we need to conduct a test later to check the compatibility with different Trino versions.

5. Change the `pom.xml` file in the `trino-gravitino` module accordingly. This is a [example](../assets/trino/pom.xml) of the `pom.xml` file in the `trino-gravitino` module.
6. Try to compile module `trino-gravitino` to see if there are any errors.
```shell
mvn clean -pl 'plugin/trino-gravitino' package -DskipTests -Dcheckstyle.skip -Dair.check.skip-checkstyle=true -DskipTests -Dair.check.skip-all=true
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to build the Trino as well

Copy link
Contributor

Choose a reason for hiding this comment

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

I have an error:

[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal io.trino:trino-maven-plugin:12:generate-service-descriptor (default-generate-service-descriptor) on project trino-graviton:
[ERROR]
[ERROR] Existing service descriptor for io.trino.spi.Plugin found in output directory.
[ERROR] -> [Help 1]

The solution is remove the file trino-connector/src/main/resources/META-INF/services/io.trino.spi.Plugin
Do you have this problem

Copy link
Contributor

Choose a reason for hiding this comment

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

you can install trino modules (exluding gravitino) at first, using:
mvn -pl '!plugin/trino-gravitino' clean install -DskipTests

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I understand that. My question is whether anyone else has encountered this error.
@yuqi1129 The steps on how to build Trino are missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@diqiu50 I have added it.
image

@yuqi1129
Copy link
Contributor Author

@diqiu50
Please help to look again.


1. Clone the Trino repository from the [GitHub](https://github.com/trinodb/trino) repository. We advise you to use the release version 426 or 435.
2. Open the Trino project in your IDEA.
3. Create a new module for the Gravitino connector in the Trino project as the following picture (we will use the name `trino-gravitino` as the module name in the following steps). ![trino-gravitino](../assets/trino/create-gravitino-connector.jpg)
Copy link
Contributor

@diqiu50 diqiu50 Mar 13, 2024

Choose a reason for hiding this comment

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

Do users know how to create a module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be tiny things compared to developing The Trino connector. If users lack this knowledge, I don't think they can develop the connector effectively.

java -jar trino-cli-429-executable.jar --server localhost:8180
```
:::note
The `trino-cli-429-executable.jar` is the Trino CLI jar file, you can download it from the [Trino release page](https://trino.io/download.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use version 429?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just an example. It works well for me with version 429, and another version might be just as good, I will add more details and let users choose the package version.

@yuqi1129
Copy link
Contributor Author

@diqiu50
I have updated the PR, please help take a look if you are free.

Copy link
Contributor

@diqiu50 diqiu50 left a comment

Choose a reason for hiding this comment

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

LGTM

@yuqi1129 yuqi1129 merged commit b154519 into apache:main Mar 14, 2024
5 checks passed
@yuqi1129 yuqi1129 deleted the issue_1193 branch March 14, 2024 03:20
yijhenlin pushed a commit to yijhenlin/gravitino that referenced this pull request Mar 14, 2024
…trino connector locally. (apache#2446)

### What changes were proposed in this pull request?

Add a document about how to debug develop and debug `trino-connector`
locally.

### Why are the changes needed?

To make users more knowledgeable about Gravitino trino connector.

Fix: apache#1193 

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

N/A.
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.

[Improvement] Add how to develop Gravitino trino connector for new developers
3 participants