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/04/24 08:42:08 UTC
[GitHub] [ignite] ivandasch opened a new pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
ivandasch opened a new pull request #7720:
URL: https://github.com/apache/ignite/pull/7720
----------------------------------------------------------------
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] ivandasch commented on a change in pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
ivandasch commented on a change in pull request #7720:
URL: https://github.com/apache/ignite/pull/7720#discussion_r416866331
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -115,4 +139,13 @@ public CacheConfigurationSplitterImpl(Marshaller marshaller) {
throw new IgniteException("Failed to serialize field [fieldName=" + fieldName + ", value=" + val + ']', e);
}
}
+
+ /**
+ * @param cfg Cache configuration.
+ * @return {@code true} If local node is affinity node for cache.
+ */
+ private boolean isAffinity(CacheConfiguration cfg) {
Review comment:
Only reasonable variant is @SuppressWarning. CacheConfiguration is generic just for compliance with jcache api, nothing more. Adding everywhere warning suppression is a concern, but at least areasonable variant. Adding everywhere wildcards is like a cargo cult.
----------------------------------------------------------------
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] xtern commented on a change in pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
xtern commented on a change in pull request #7720:
URL: https://github.com/apache/ignite/pull/7720#discussion_r416846406
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -41,11 +44,32 @@
/** Marshaller. */
private final Marshaller marshaller;
+ /** Grid kernal context. */
+ private final GridKernalContext ctx;
+
/**
* @param marshaller Marshaller.
+ * @param ctx Grid kernal context.
*/
- public CacheConfigurationSplitterImpl(Marshaller marshaller) {
+ public CacheConfigurationSplitterImpl(Marshaller marshaller, GridKernalContext ctx) {
this.marshaller = marshaller;
Review comment:
ok, it's just a suggestion.
For me It seems that the code with the assertions is more concise and readable and I don't see any problem with "bloated bytecode".
----------------------------------------------------------------
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] xtern commented on a change in pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
xtern commented on a change in pull request #7720:
URL: https://github.com/apache/ignite/pull/7720#discussion_r416851523
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -115,4 +139,13 @@ public CacheConfigurationSplitterImpl(Marshaller marshaller) {
throw new IgniteException("Failed to serialize field [fieldName=" + fieldName + ", value=" + val + ']', e);
}
}
+
+ /**
+ * @param cfg Cache configuration.
+ * @return {@code true} If local node is affinity node for cache.
+ */
+ private boolean isAffinity(CacheConfiguration cfg) {
Review comment:
I suggest to fix this warning somehow, because:
1. it's java compiler warning
2. most people use an IDE that will highlight it.
----------------------------------------------------------------
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] zstan commented on a change in pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #7720:
URL: https://github.com/apache/ignite/pull/7720#discussion_r417064117
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -41,11 +44,32 @@
/** Marshaller. */
private final Marshaller marshaller;
+ /** Grid kernal context. */
+ private final GridKernalContext ctx;
+
/**
* @param marshaller Marshaller.
+ * @param ctx Grid kernal context.
*/
- public CacheConfigurationSplitterImpl(Marshaller marshaller) {
+ public CacheConfigurationSplitterImpl(Marshaller marshaller, GridKernalContext ctx) {
this.marshaller = marshaller;
Review comment:
+1 with Ivan here, if it`s necessary u may add @Nullable and check them, but not asserts.
----------------------------------------------------------------
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] xtern commented on a change in pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
xtern commented on a change in pull request #7720:
URL: https://github.com/apache/ignite/pull/7720#discussion_r416851523
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -115,4 +139,13 @@ public CacheConfigurationSplitterImpl(Marshaller marshaller) {
throw new IgniteException("Failed to serialize field [fieldName=" + fieldName + ", value=" + val + ']', e);
}
}
+
+ /**
+ * @param cfg Cache configuration.
+ * @return {@code true} If local node is affinity node for cache.
+ */
+ private boolean isAffinity(CacheConfiguration cfg) {
Review comment:
I suggest to fix this warning somehow, because:
1. it's java compiler warning
2. most people use an IDE that will highlight this code.
3. we trying to cleanup Ignite code.
----------------------------------------------------------------
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] xtern commented on a change in pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
xtern commented on a change in pull request #7720:
URL: https://github.com/apache/ignite/pull/7720#discussion_r416851523
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -115,4 +139,13 @@ public CacheConfigurationSplitterImpl(Marshaller marshaller) {
throw new IgniteException("Failed to serialize field [fieldName=" + fieldName + ", value=" + val + ']', e);
}
}
+
+ /**
+ * @param cfg Cache configuration.
+ * @return {@code true} If local node is affinity node for cache.
+ */
+ private boolean isAffinity(CacheConfiguration cfg) {
Review comment:
1. Because it's java compiler warning
2. Because most people use an IDE that will highlight this code.
3. Because we trying to cleanup Ignite code.
----------------------------------------------------------------
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] xtern commented on a change in pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
xtern commented on a change in pull request #7720:
URL: https://github.com/apache/ignite/pull/7720#discussion_r416851523
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -115,4 +139,13 @@ public CacheConfigurationSplitterImpl(Marshaller marshaller) {
throw new IgniteException("Failed to serialize field [fieldName=" + fieldName + ", value=" + val + ']', e);
}
}
+
+ /**
+ * @param cfg Cache configuration.
+ * @return {@code true} If local node is affinity node for cache.
+ */
+ private boolean isAffinity(CacheConfiguration cfg) {
Review comment:
I suggest to fix this warning somehow, because:
1. it's java compiler warning
2. most people use an IDE that will highlight this code.
----------------------------------------------------------------
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] asfgit closed pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #7720:
URL: https://github.com/apache/ignite/pull/7720
----------------------------------------------------------------
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] ivandasch commented on a change in pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
ivandasch commented on a change in pull request #7720:
URL: https://github.com/apache/ignite/pull/7720#discussion_r416841110
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -115,4 +139,13 @@ public CacheConfigurationSplitterImpl(Marshaller marshaller) {
throw new IgniteException("Failed to serialize field [fieldName=" + fieldName + ", value=" + val + ']', e);
}
}
+
+ /**
+ * @param cfg Cache configuration.
+ * @return {@code true} If local node is affinity node for cache.
+ */
+ private boolean isAffinity(CacheConfiguration cfg) {
Review comment:
I don't see any reason why I should add wildcard. Wildcards and bounds are useful when we deal with covariance or contravariance. This is not a case. Moreover, there is not any wildcard in this class, why I should add one? No reason at all, except highliting in one well-known IDE
----------------------------------------------------------------
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] ivandasch commented on a change in pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
ivandasch commented on a change in pull request #7720:
URL: https://github.com/apache/ignite/pull/7720#discussion_r416829174
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -41,11 +44,32 @@
/** Marshaller. */
private final Marshaller marshaller;
+ /** Grid kernal context. */
+ private final GridKernalContext ctx;
+
/**
* @param marshaller Marshaller.
+ * @param ctx Grid kernal context.
*/
- public CacheConfigurationSplitterImpl(Marshaller marshaller) {
+ public CacheConfigurationSplitterImpl(Marshaller marshaller, GridKernalContext ctx) {
this.marshaller = marshaller;
Review comment:
I strongly disagree. I am against this unnecessary asserts, this leads to bloated bytecode, nothing more
----------------------------------------------------------------
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] xtern commented on a change in pull request #7720: IGNITE-12898 Fix incorrect collecting cache configuration (missing CacheConfigurationEnrichment) on coordinator.
Posted by GitBox <gi...@apache.org>.
xtern commented on a change in pull request #7720:
URL: https://github.com/apache/ignite/pull/7720#discussion_r416777266
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -41,11 +44,32 @@
/** Marshaller. */
private final Marshaller marshaller;
+ /** Grid kernal context. */
+ private final GridKernalContext ctx;
+
/**
* @param marshaller Marshaller.
+ * @param ctx Grid kernal context.
*/
- public CacheConfigurationSplitterImpl(Marshaller marshaller) {
+ public CacheConfigurationSplitterImpl(Marshaller marshaller, GridKernalContext ctx) {
this.marshaller = marshaller;
Review comment:
let's add assertions for arguments
assert marshaller != null;
assert ctx != null;
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheConfigurationSplitterImpl.java
##########
@@ -115,4 +139,13 @@ public CacheConfigurationSplitterImpl(Marshaller marshaller) {
throw new IgniteException("Failed to serialize field [fieldName=" + fieldName + ", value=" + val + ']', e);
}
}
+
+ /**
+ * @param cfg Cache configuration.
+ * @return {@code true} If local node is affinity node for cache.
+ */
+ private boolean isAffinity(CacheConfiguration cfg) {
Review comment:
Let's change to CacheConfiguration<?, ?> to prevent "unchecked" warning.
----------------------------------------------------------------
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