You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "Mmuzaf (via GitHub)" <gi...@apache.org> on 2023/12/01 18:38:24 UTC
[PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Mmuzaf opened a new pull request, #2958:
URL: https://github.com/apache/cassandra/pull/2958
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) 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](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
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](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1459777809
##########
src/java/org/apache/cassandra/db/virtual/proc/SystemViewAnnotationProcessor.java:
##########
Review Comment:
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/` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1467771414
##########
build.xml:
##########
@@ -478,6 +481,35 @@
</exec>
</target>
+ <!-- Include annotation processor for VirtualTable -->
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1500864819
##########
src/java/org/apache/cassandra/db/virtual/BatchMetricsTable.java:
##########
@@ -61,6 +67,8 @@ public DataSet data()
addRow(result, PARTITIONS_PER_UNLOGGED_BATCH, metrics.partitionsPerUnloggedBatch.getSnapshot());
addRow(result, PARTITIONS_PER_COUNTER_BATCH, metrics.partitionsPerCounterBatch.getSnapshot());
+ ClientWarn.instance.warn("The \"batch_metrics\" virtual table is deprecated. " +
Review Comment:
nit: constants for keyspace / table, if they exist, might be used
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1460001543
##########
src/java/org/apache/cassandra/db/virtual/proc/SystemViewAnnotationProcessor.java:
##########
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1461065424
##########
src/java/org/apache/cassandra/db/virtual/proc/SystemViewAnnotationProcessor.java:
##########
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1467773666
##########
src/java/org/apache/cassandra/db/virtual/proc/SystemViewAnnotationProcessor.java:
##########
Review Comment:
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, since it's neither the `tools` nor the `tests`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "pmcfadin (via GitHub)" <gi...@apache.org>.
pmcfadin closed pull request #2958: CASSANDRA-14572 Expose all table metrics in virtual tables
URL: https://github.com/apache/cassandra/pull/2958
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1459777809
##########
src/java/org/apache/cassandra/db/virtual/proc/SystemViewAnnotationProcessor.java:
##########
Review Comment:
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/` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1459781721
##########
src/resources/META-INF/services/javax.annotation.processing.Processor:
##########
Review Comment:
under maybe `test/annotation-processor/resources/` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1467773666
##########
src/java/org/apache/cassandra/db/virtual/proc/SystemViewAnnotationProcessor.java:
##########
Review Comment:
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, since it's neither the `tools` nor the `tests`.
##########
src/resources/META-INF/services/javax.annotation.processing.Processor:
##########
Review Comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1467773666
##########
src/java/org/apache/cassandra/db/virtual/proc/SystemViewAnnotationProcessor.java:
##########
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1467771150
##########
build.xml:
##########
@@ -478,6 +481,35 @@
</exec>
</target>
+ <!-- Include annotation processor for VirtualTable -->
+ <property name="processor.build.classes" value="${build.classes}/processor" />
+ <property name="processor.manifest" value="${processor.build.classes}/MANIFEST.MF" />
+
+ <target name="annotation-processor-build" depends="init" description="Build system view annotation processor">
+ <antcall target="_annotation_processor_build"/>
+ </target>
+
+ <target name="_annotation_processor_build">
+ <mkdir dir="${processor.build.classes}" />
+ <javac includes="**/virtual/proc/*.java"
+ destdir="${processor.build.classes}"
+ includeantruntime="true"
+ source="${java.default}" target="${java.default}">
+ <src path="${build.src}"/>
+ </javac>
+ </target>
+
+ <target name="annotation-processor-jar"
+ depends="annotation-processor-build">
+ <mkdir dir="${processor.build.classes}/META-INF" />
+ <jar destfile="${build.dir.lib}/jars/cassandra-annotation-processor-${version}.jar">
+ <fileset dir="${processor.build.classes}"/>
+ <manifest>
+ <attribute name="Built-By" value="Maxim Muzafarov"/>
Review Comment:
I did it the same way with the other stuff in the build.xml, but yeah it looks weird. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1459751801
##########
build.xml:
##########
@@ -478,6 +481,35 @@
</exec>
</target>
+ <!-- Include annotation processor for VirtualTable -->
+ <property name="processor.build.classes" value="${build.classes}/processor" />
+ <property name="processor.manifest" value="${processor.build.classes}/MANIFEST.MF" />
+
+ <target name="annotation-processor-build" depends="init" description="Build system view annotation processor">
+ <antcall target="_annotation_processor_build"/>
+ </target>
+
+ <target name="_annotation_processor_build">
+ <mkdir dir="${processor.build.classes}" />
+ <javac includes="**/virtual/proc/*.java"
+ destdir="${processor.build.classes}"
+ includeantruntime="true"
+ source="${java.default}" target="${java.default}">
+ <src path="${build.src}"/>
+ </javac>
+ </target>
+
+ <target name="annotation-processor-jar"
+ depends="annotation-processor-build">
+ <mkdir dir="${processor.build.classes}/META-INF" />
+ <jar destfile="${build.dir.lib}/jars/cassandra-annotation-processor-${version}.jar">
+ <fileset dir="${processor.build.classes}"/>
+ <manifest>
+ <attribute name="Built-By" value="Maxim Muzafarov"/>
Review Comment:
is this needed ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#discussion_r1459776177
##########
build.xml:
##########
@@ -478,6 +481,35 @@
</exec>
</target>
+ <!-- Include annotation processor for VirtualTable -->
Review Comment:
encapsulate this annotation processor to `.build/build-compile-annotations.xml` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-14572 Expose all table metrics in virtual tables [cassandra]
Posted by "pmcfadin (via GitHub)" <gi...@apache.org>.
pmcfadin commented on PR #2958:
URL: https://github.com/apache/cassandra/pull/2958#issuecomment-4227139110
Closing as [CASSANDRA-14572](https://issues.apache.org/jira/browse/CASSANDRA-14572) has been resolved as Fixed — if this PR contains changes not captured in the merged fix, please reopen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org