[GitHub] [ignite] agura commented on a change in pull request #8135: IGNITE-12921: SqlViewExporterSpi moved to the internal package.

[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