You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/08/10 12:23:38 UTC
[GitHub] [ignite] nizhikov opened a new pull request #8135: IGNITE-12921: SqlViewExporterSpi moved to the internal package.
nizhikov opened a new pull request #8135:
URL: https://github.com/apache/ignite/pull/8135
SqlViewExporterSpi moved to the internal package to eliminate it from public API.
Instances of this SPI will be created automatically in case the indexing module available during node startup.
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] agura commented on a change in pull request #8135: IGNITE-12921: SqlViewExporterSpi moved to the internal package.
Posted by GitBox <gi...@apache.org>.
agura commented on a change in pull request #8135:
URL: https://github.com/apache/ignite/pull/8135#discussion_r471698263
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/systemview/GridSystemViewManager.java
##########
@@ -53,6 +57,9 @@
*/
public class GridSystemViewManager extends GridManagerAdapter<SystemViewExporterSpi>
implements ReadOnlySystemViewRegistry {
+ /** Class name for a SQL view exporter of system views. */
+ public static final String SYSTEM_VIEW_SQL_SPI = "org.apache.ignite.internal.managers.systemview.SqlViewExporterSpi";
Review comment:
Visibility scope for this constant is too broad.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/systemview/GridSystemViewManager.java
##########
@@ -75,7 +82,26 @@
* @param ctx Kernal context.
*/
public GridSystemViewManager(GridKernalContext ctx) {
- super(ctx, ctx.config().getSystemViewExporterSpi());
+ super(ctx, ((Supplier<SystemViewExporterSpi[]>)() -> {
Review comment:
Could you please replace supplier by simple method call? There is no need to use lambda syntax here.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/systemview/GridSystemViewManager.java
##########
@@ -75,7 +82,26 @@
* @param ctx Kernal context.
*/
public GridSystemViewManager(GridKernalContext ctx) {
- super(ctx, ctx.config().getSystemViewExporterSpi());
+ super(ctx, ((Supplier<SystemViewExporterSpi[]>)() -> {
+ SystemViewExporterSpi[] spi = ctx.config().getSystemViewExporterSpi();
+
+ if (!IgniteComponentType.INDEXING.inClassPath())
+ return spi;
+
+ SystemViewExporterSpi[] spiWithSql = new SystemViewExporterSpi[spi != null ? spi.length + 1 : 1];
Review comment:
Nevertheless, what if `spi` array already contains `SqlViewExporterSpi`?
##########
File path: modules/indexing/src/main/java/org/apache/ignite/internal/managers/systemview/SqlViewExporterSpi.java
##########
@@ -36,12 +37,9 @@
* This SPI implementation exports metrics as SQL views.
*
* Note, instance of this class created with reflection.
- * @see IgnitionEx#SYSTEM_VIEW_SQL_SPI
+ * @see GridSystemViewManager#SYSTEM_VIEW_SQL_SPI
*/
-public class SqlViewExporterSpi extends IgniteSpiAdapter implements SystemViewExporterSpi {
- /** System view filter. */
- @Nullable private Predicate<SystemView<?>> filter;
-
+class SqlViewExporterSpi extends IgniteSpiAdapter implements SystemViewExporterSpi {
Review comment:
IMHO, it isn't the best solution to have internal implementation of any SPI. May be we can extract some common parts of Exporter SPI into a new internal super-class which should be inherited by `SystemViewExporterSpi` and our internal analog of `SqlViewEXporterSpi`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a change in pull request #8135: IGNITE-12921 SqlViewExporterSpi moved to the internal package
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8135:
URL: https://github.com/apache/ignite/pull/8135#discussion_r471972085
##########
File path: modules/indexing/src/main/java/org/apache/ignite/internal/managers/systemview/SqlViewExporterSpi.java
##########
@@ -36,12 +37,9 @@
* This SPI implementation exports metrics as SQL views.
*
* Note, instance of this class created with reflection.
- * @see IgnitionEx#SYSTEM_VIEW_SQL_SPI
+ * @see GridSystemViewManager#SYSTEM_VIEW_SQL_SPI
*/
-public class SqlViewExporterSpi extends IgniteSpiAdapter implements SystemViewExporterSpi {
- /** System view filter. */
- @Nullable private Predicate<SystemView<?>> filter;
-
+class SqlViewExporterSpi extends IgniteSpiAdapter implements SystemViewExporterSpi {
Review comment:
`AbstractSystemViewExporterSpi` introduced. Please, take a look one more time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] agura commented on a change in pull request #8135: IGNITE-12921 SqlViewExporterSpi moved to the internal package
Posted by GitBox <gi...@apache.org>.
agura commented on a change in pull request #8135:
URL: https://github.com/apache/ignite/pull/8135#discussion_r490202091
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/systemview/GridSystemViewManager.java
##########
@@ -75,7 +82,26 @@
* @param ctx Kernal context.
*/
public GridSystemViewManager(GridKernalContext ctx) {
- super(ctx, ctx.config().getSystemViewExporterSpi());
+ super(ctx, ((Supplier<SystemViewExporterSpi[]>)() -> {
+ SystemViewExporterSpi[] spi = ctx.config().getSystemViewExporterSpi();
+
+ if (!IgniteComponentType.INDEXING.inClassPath())
+ return spi;
+
+ SystemViewExporterSpi[] spiWithSql = new SystemViewExporterSpi[spi != null ? spi.length + 1 : 1];
Review comment:
It's definitely redundant deu to a package privacy. I told about naming only. Please revert this check and merge.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/systemview/GridSystemViewManager.java
##########
@@ -75,7 +82,26 @@
* @param ctx Kernal context.
*/
public GridSystemViewManager(GridKernalContext ctx) {
- super(ctx, ctx.config().getSystemViewExporterSpi());
+ super(ctx, ((Supplier<SystemViewExporterSpi[]>)() -> {
+ SystemViewExporterSpi[] spi = ctx.config().getSystemViewExporterSpi();
+
+ if (!IgniteComponentType.INDEXING.inClassPath())
+ return spi;
+
+ SystemViewExporterSpi[] spiWithSql = new SystemViewExporterSpi[spi != null ? spi.length + 1 : 1];
Review comment:
It's definitely redundant due to a package privacy. I told about naming only. Please revert this check and merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a change in pull request #8135: IGNITE-12921 SqlViewExporterSpi moved to the internal package
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8135:
URL: https://github.com/apache/ignite/pull/8135#discussion_r490133937
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/systemview/GridSystemViewManager.java
##########
@@ -75,7 +82,26 @@
* @param ctx Kernal context.
*/
public GridSystemViewManager(GridKernalContext ctx) {
- super(ctx, ctx.config().getSystemViewExporterSpi());
+ super(ctx, ((Supplier<SystemViewExporterSpi[]>)() -> {
+ SystemViewExporterSpi[] spi = ctx.config().getSystemViewExporterSpi();
+
+ if (!IgniteComponentType.INDEXING.inClassPath())
+ return spi;
+
+ SystemViewExporterSpi[] spiWithSql = new SystemViewExporterSpi[spi != null ? spi.length + 1 : 1];
Review comment:
OK, I've added a check to ensure `SqlViewExporterSpi` not presented in the initial array.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a change in pull request #8135: IGNITE-12921 SqlViewExporterSpi moved to the internal package
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8135:
URL: https://github.com/apache/ignite/pull/8135#discussion_r471956931
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/systemview/GridSystemViewManager.java
##########
@@ -75,7 +82,26 @@
* @param ctx Kernal context.
*/
public GridSystemViewManager(GridKernalContext ctx) {
- super(ctx, ctx.config().getSystemViewExporterSpi());
+ super(ctx, ((Supplier<SystemViewExporterSpi[]>)() -> {
+ SystemViewExporterSpi[] spi = ctx.config().getSystemViewExporterSpi();
+
+ if (!IgniteComponentType.INDEXING.inClassPath())
+ return spi;
+
+ SystemViewExporterSpi[] spiWithSql = new SystemViewExporterSpi[spi != null ? spi.length + 1 : 1];
Review comment:
`SqlViewExporterSpi` now is package-private so it's forbidden for the user to create instances.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov merged pull request #8135: IGNITE-12921 SqlViewExporterSpi moved to the internal package
Posted by GitBox <gi...@apache.org>.
nizhikov merged pull request #8135:
URL: https://github.com/apache/ignite/pull/8135
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a change in pull request #8135: IGNITE-12921 SqlViewExporterSpi moved to the internal package
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8135:
URL: https://github.com/apache/ignite/pull/8135#discussion_r471962415
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/systemview/GridSystemViewManager.java
##########
@@ -53,6 +57,9 @@
*/
public class GridSystemViewManager extends GridManagerAdapter<SystemViewExporterSpi>
implements ReadOnlySystemViewRegistry {
+ /** Class name for a SQL view exporter of system views. */
+ public static final String SYSTEM_VIEW_SQL_SPI = "org.apache.ignite.internal.managers.systemview.SqlViewExporterSpi";
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] agura commented on a change in pull request #8135: IGNITE-12921 SqlViewExporterSpi moved to the internal package
Posted by GitBox <gi...@apache.org>.
agura commented on a change in pull request #8135:
URL: https://github.com/apache/ignite/pull/8135#discussion_r489079912
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/systemview/GridSystemViewManager.java
##########
@@ -75,7 +82,26 @@
* @param ctx Kernal context.
*/
public GridSystemViewManager(GridKernalContext ctx) {
- super(ctx, ctx.config().getSystemViewExporterSpi());
+ super(ctx, ((Supplier<SystemViewExporterSpi[]>)() -> {
+ SystemViewExporterSpi[] spi = ctx.config().getSystemViewExporterSpi();
+
+ if (!IgniteComponentType.INDEXING.inClassPath())
+ return spi;
+
+ SystemViewExporterSpi[] spiWithSql = new SystemViewExporterSpi[spi != null ? spi.length + 1 : 1];
Review comment:
Any way it is just somewhat sloppy.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a change in pull request #8135: IGNITE-12921 SqlViewExporterSpi moved to the internal package
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8135:
URL: https://github.com/apache/ignite/pull/8135#discussion_r471961931
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/systemview/GridSystemViewManager.java
##########
@@ -75,7 +82,26 @@
* @param ctx Kernal context.
*/
public GridSystemViewManager(GridKernalContext ctx) {
- super(ctx, ctx.config().getSystemViewExporterSpi());
+ super(ctx, ((Supplier<SystemViewExporterSpi[]>)() -> {
Review comment:
replaced.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] agura commented on pull request #8135: IGNITE-12921: SqlViewExporterSpi moved to the internal package.
Posted by GitBox <gi...@apache.org>.
agura commented on pull request #8135:
URL: https://github.com/apache/ignite/pull/8135#issuecomment-675052684
Please, remove `:` after issue number in commit message. See the contribution checklist for details.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on pull request #8135: IGNITE-12921: SqlViewExporterSpi moved to the internal package.
Posted by GitBox <gi...@apache.org>.
nizhikov commented on pull request #8135:
URL: https://github.com/apache/ignite/pull/8135#issuecomment-671323799
Hello, @agura can you please, take a look at my changes?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org