ibessonov commented on code in PR #2580:
URL: https://github.com/apache/ignite-3/pull/2580#discussion_r1324063804
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationTreeGenerator.java:
##########
@@ -161,12 +160,14 @@ public synchronized void close() {
* @param polymorphicSchemaExtensions polymorphic schema extensions
* @return set of all schema classes
*/
- private static Set<Class<?>> collectAllSchemas(Collection<RootKey<?, ?>>
rootKeys,
+ private static Set<Class<?>> collectAllSchemas(
+ Collection<RootKey<?, ?>> rootKeys,
Collection<Class<?>> internalSchemaExtensions,
- Collection<Class<?>> polymorphicSchemaExtensions) {
+ Collection<Class<?>> polymorphicSchemaExtensions
+ ) {
Set<Class<?>> allSchemas = new HashSet<>();
- allSchemas.addAll(collectSchemas(viewReadOnly(rootKeys,
RootKey::schemaClass)));
+ allSchemas.addAll(collectSchemas(() ->
ConfigurationUtil.mapIterator(rootKeys.iterator(), RootKey::schemaClass)));
Review Comment:
Please use static import
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/notifications/ConfigurationNotificationUtils.java:
##########
@@ -121,12 +118,29 @@ static <T> Iterator<ConfigurationNamedListListener<T>>
extendedListeners(
* @param anyConfig New {@link NamedListConfiguration#any "any"}
configuration.
* @return Merged {@link NamedListConfiguration#any "any"} configurations.
*/
- static Collection<DynamicConfiguration<InnerNode, ?>> mergeAnyConfigs(
- Collection<DynamicConfiguration<InnerNode, ?>> anyConfigs,
+ static Iterator<DynamicConfiguration<InnerNode, ?>> mergeAnyConfigs(
+ Iterator<DynamicConfiguration<InnerNode, ?>> anyConfigs,
@Nullable DynamicConfiguration<InnerNode, ?> anyConfig
) {
- return Stream.concat(anyConfigs.stream(), Stream.of(anyConfig))
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ return new Iterator<>() {
+ @Override
+ public boolean hasNext() {
+ return anyConfigs.hasNext() || anyConfig != null;
+
+ }
+
+ @Override
+ public DynamicConfiguration<InnerNode, ?> next() {
+ if (anyConfigs.hasNext()) {
+ return anyConfigs.next();
+ }
+
+ if (anyConfig == null) {
+ throw new NoSuchElementException();
+ }
Review Comment:
Should we replace this with an assertion? It should never be null under
normal circumstances.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java:
##########
@@ -886,6 +889,120 @@ public static void touch(DynamicConfiguration<?, ?> cfg) {
}
}
+ /**
+ * Map iterable via provided mapper function. Filter all {@code null}
mapped values.
+ *
+ * @param iterator Basic iterator.
+ * @param mapper Conversion function.
+ * @param <T1> Base type of the collection.
+ * @param <T2> Type for view.
+ * @return Mapped iterable.
+ */
+ public static <T1, T2> Iterator<T2> mapIterator(
+ @Nullable Iterator<? extends T1> iterator,
+ @Nullable Function<? super T1, ? extends T2> mapper
+ ) {
+ if (iterator == null) {
+ return Collections.emptyIterator();
+ }
+
+ if (mapper == null) {
+ return (Iterator<T2>) iterator;
+ }
+
+ return new Iterator<>() {
+ @Nullable
+ private T2 next;
+
+ @Override
+ public boolean hasNext() {
+ if (!iterator.hasNext()) {
+ return false;
+ }
+
+ if (next == null) {
+ next = mapper.apply(iterator.next());
+ if (next == null) {
+ return hasNext();
+ }
+ }
+
+ return true;
+ }
+
+ @Override
+ public T2 next() {
+ if (next == null) {
+ throw new NoSuchElementException();
+ }
+ T2 result = next;
+ next = null;
+ return result;
+ }
+ };
+ }
+
+ /**
+ * Map iterator via provided mapper function. Filter all {@code null}
mapped values.
Review Comment:
```suggestion
* Maps iterator via provided mapper function. Filter all {@code null}
mapped values.
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java:
##########
@@ -886,6 +889,120 @@ public static void touch(DynamicConfiguration<?, ?> cfg) {
}
}
+ /**
+ * Map iterable via provided mapper function. Filter all {@code null}
mapped values.
+ *
+ * @param iterator Basic iterator.
+ * @param mapper Conversion function.
+ * @param <T1> Base type of the collection.
+ * @param <T2> Type for view.
+ * @return Mapped iterable.
+ */
+ public static <T1, T2> Iterator<T2> mapIterator(
+ @Nullable Iterator<? extends T1> iterator,
+ @Nullable Function<? super T1, ? extends T2> mapper
+ ) {
+ if (iterator == null) {
+ return Collections.emptyIterator();
+ }
+
+ if (mapper == null) {
+ return (Iterator<T2>) iterator;
+ }
+
+ return new Iterator<>() {
+ @Nullable
+ private T2 next;
+
+ @Override
+ public boolean hasNext() {
+ if (!iterator.hasNext()) {
+ return false;
+ }
+
+ if (next == null) {
+ next = mapper.apply(iterator.next());
+ if (next == null) {
+ return hasNext();
+ }
+ }
+
+ return true;
+ }
+
+ @Override
+ public T2 next() {
+ if (next == null) {
+ throw new NoSuchElementException();
+ }
+ T2 result = next;
+ next = null;
+ return result;
+ }
+ };
+ }
+
+ /**
+ * Map iterator via provided mapper function. Filter all {@code null}
mapped values.
Review Comment:
What's up with "null mapped values"? It doesn't filter them
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java:
##########
@@ -886,6 +889,120 @@ public static void touch(DynamicConfiguration<?, ?> cfg) {
}
}
+ /**
+ * Map iterable via provided mapper function. Filter all {@code null}
mapped values.
+ *
+ * @param iterator Basic iterator.
+ * @param mapper Conversion function.
+ * @param <T1> Base type of the collection.
+ * @param <T2> Type for view.
+ * @return Mapped iterable.
+ */
+ public static <T1, T2> Iterator<T2> mapIterator(
+ @Nullable Iterator<? extends T1> iterator,
+ @Nullable Function<? super T1, ? extends T2> mapper
+ ) {
+ if (iterator == null) {
+ return Collections.emptyIterator();
+ }
+
+ if (mapper == null) {
+ return (Iterator<T2>) iterator;
+ }
+
+ return new Iterator<>() {
+ @Nullable
+ private T2 next;
+
+ @Override
+ public boolean hasNext() {
+ if (!iterator.hasNext()) {
+ return false;
+ }
+
+ if (next == null) {
+ next = mapper.apply(iterator.next());
+ if (next == null) {
+ return hasNext();
Review Comment:
Please re-write this code using loop :)
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/store/GroupPageStoresMap.java:
##########
@@ -90,7 +90,7 @@ public boolean contains(GroupPartitionId groupPartitionId) {
* Returns a view of all page stores of all groups.
*/
public Collection<GroupPartitionPageStore<T>> getAll() {
- return
CollectionUtils.viewReadOnly(groupPartitionIdPageStore.entrySet(),
GroupPartitionPageStore::new);
+ return
groupPartitionIdPageStore.entrySet().stream().map(GroupPartitionPageStore::new).collect(Collectors.toList());
Review Comment:
This part was explicitly written in this way to avoid allocating new
collection. Please make this method return Stream, for example. It's on the hot
path.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java:
##########
@@ -886,6 +889,120 @@ public static void touch(DynamicConfiguration<?, ?> cfg) {
}
}
+ /**
+ * Map iterable via provided mapper function. Filter all {@code null}
mapped values.
+ *
+ * @param iterator Basic iterator.
+ * @param mapper Conversion function.
+ * @param <T1> Base type of the collection.
+ * @param <T2> Type for view.
+ * @return Mapped iterable.
+ */
+ public static <T1, T2> Iterator<T2> mapIterator(
+ @Nullable Iterator<? extends T1> iterator,
+ @Nullable Function<? super T1, ? extends T2> mapper
+ ) {
+ if (iterator == null) {
+ return Collections.emptyIterator();
+ }
+
+ if (mapper == null) {
+ return (Iterator<T2>) iterator;
+ }
+
+ return new Iterator<>() {
+ @Nullable
+ private T2 next;
+
+ @Override
+ public boolean hasNext() {
+ if (!iterator.hasNext()) {
+ return false;
+ }
+
+ if (next == null) {
+ next = mapper.apply(iterator.next());
+ if (next == null) {
+ return hasNext();
+ }
+ }
+
+ return true;
+ }
+
+ @Override
+ public T2 next() {
+ if (next == null) {
+ throw new NoSuchElementException();
+ }
+ T2 result = next;
+ next = null;
+ return result;
+ }
+ };
+ }
+
+ /**
+ * Map iterator via provided mapper function. Filter all {@code null}
mapped values.
+ *
+ * @param iterator Basic iterator.
+ * @param mapper Conversion function.
+ * @param predicate Predicate to apply to each element of basic collection.
+ * @param <T1> Base type of the collection.
+ * @param <T2> Type for view.
+ * @return Read-only collection view.
Review Comment:
This part is also not updated
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/util/ConfigurationUtilTest.java:
##########
@@ -1117,6 +1125,86 @@ void testAddDefaultsPurity() {
assertNotSame(afterDefaults, beforeDefaults);
}
+ /**
+ * Collect of elements.
+ *
+ * @param iterable Iterable.
+ * @param <T> Type of the elements.
+ * @return Collected elements.
+ */
+ private <T> List<? extends T> collect(Iterator<? extends T> iterable) {
+ return
StreamSupport.stream(Spliterators.spliteratorUnknownSize(iterable, 0),
false).collect(toList());
+ }
+
+ @Test
+ void testViewReadOnly() {
+ assertFalse(ConfigurationUtil.mapIterator(null, null).hasNext());
Review Comment:
Please use static imports here as well
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
