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

CASSANDRA-14572 Expose all table metrics in virtual tables #2958

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

Mmuzaf
Copy link
Contributor

@Mmuzaf Mmuzaf commented Dec 1, 2023

Thanks for sending a pull request! Here are some tips if you're new here:

  • Ensure you have added or run the appropriate tests for your PR.
  • Be sure to keep the PR description updated to reflect all changes.
  • Write your PR title to summarize what this PR proposes.
  • If possible, provide a concise example to reproduce the issue for a faster review.
  • Read our contributor guidelines
  • If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

build.xml Outdated Show resolved Hide resolved
build.xml Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

can we not have this java file under src/java/ please.

like the test/anttasks/org/apache/cassandra/anttasks/ files, a separate directory structure would be nice (intuitive it's a separate compile and run classpath)

no good suggestions, maybe test/annotation-processor/src/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While the SystemViewAnnotationProcessor reduces the need for boilerplate code for some additional model classes, it does add complexity to the build scripts. This is a trade-off that we need to agree on.

For me, keeping all the generated classes sounds slightly better than increasing the complexity of the build, as the build.xml is already quite large. Do you think it's better to remove the SystemViewAnnotationProcessor or is it better to keep it and move it to the path you've suggested?

The rationale is here, option #1 from here.
https://issues.apache.org/jira/browse/CASSANDRA-14572?focusedCommentId=17800171&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17800171

Copy link
Member

Choose a reason for hiding this comment

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

I'm +0 towards not keeping the generated classes. And the build complexity can hopefully be all encapsulated in a separate .build/build-compile-annotations.xml file.

If it is kept, then keep it elsewhere than the main src/java/ please.

Copy link
Contributor Author

@Mmuzaf Mmuzaf Jan 26, 2024

Choose a reason for hiding this comment

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

Ok, let's leave it as is for now. I moved it to test/annotation-processor, but we don't have a good place for it, as it's neither the tools nor the tests are suitable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants