[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323587227
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,45 +378,116 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+public void list(String name, String
description,
+Class rowClazz, Supplier> data, Function
rowFunc, Consumer clearer) {
+
+Supplier> listCreator = () -> list(name, () -> new
MonitoringListAdapter<>(name,
+description,
+rowClazz,
+(MonitoringRowAttributeWalker)walkers.get(rowClazz),
+data.get(),
+rowFunc));
+
+//Create new instance of the list.
+listCreator.get();
+
+ctx.metric().addEnableListListener(listenOnlyEqual(name, identity(), n
-> listCreator.get()));
Review comment:
I don't say that I didn't understand the code, I say that code is hard to
understand because it uses 5-6 nested lambdas. It's certainly not a pretty
simple.
I don't propose to rename it to "list enabler", I say that "listener" is a
bad abstraction for this case.
Perhaps code can be simplified in this way:
1. Create maps for list creators and removers (name -> closure) instead of
lists for listeners
2. On enabling/disabling just call closure from this map by name.
3. Get rid of as most nested lambdas as possible. Now it looks redundant.
Ideally, for list creation case there should be only two labdas: one for list
creation closure, and one for `computeIfAbsent` mapping function.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323589116
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/list/MonitoringListAdapter.java
##
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.metric.list;
+
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Function;
+import org.apache.ignite.spi.metric.list.MonitoringRow;
+import org.apache.ignite.spi.metric.list.MonitoringRowAttributeWalker;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Monitoring list backed by {@code data} {@link ConcurrentMap}.
+ */
+public class MonitoringListAdapter extends
AbstractMonitoringList {
+/** Data backed by this list. */
+private final ConcurrentMap data;
Review comment:
You use only `values()` collection from this map. I see no reason to pass
map here.
And `Map` say nothing about thread safety. Thread safety provides by the
implementation, not by the interface.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323591133
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,45 +378,116 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+public void list(String name, String
description,
+Class rowClazz, Supplier> data, Function
rowFunc, Consumer clearer) {
+
+Supplier> listCreator = () -> list(name, () -> new
MonitoringListAdapter<>(name,
+description,
+rowClazz,
+(MonitoringRowAttributeWalker)walkers.get(rowClazz),
+data.get(),
+rowFunc));
+
+//Create new instance of the list.
+listCreator.get();
+
+ctx.metric().addEnableListListener(listenOnlyEqual(name, identity(), n
-> listCreator.get()));
Review comment:
> Create maps for list creators and removers (name -> closure) instead of
lists for listeners
Yes, we can do it.
But:
1. This will require one more additional listener map because we already
have 'List' of the listeners that comes from `MonitoringExporterSpi`. Please,
see `ReadOnlyMonitoringListRegistry#addListRemoveListener`.
2. I expect very few enable/disable events, and from 10 to 20 lists over
Ignite, so we shouldn't observe performance issues because of name check in
`listenOnlyEqual`.
So I prefer to keep it as is.
What do you think?
> Get rid of as most nested lambdas as possible
Seems, I'm already did it. Can you see, how lambdas can be reduced?
What is the issue with the lambdas count, in the first place?
Do you see some issues with code readability, understandability?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323591700
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,45 +378,116 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+public void list(String name, String
description,
+Class rowClazz, Supplier> data, Function
rowFunc, Consumer clearer) {
+
+Supplier> listCreator = () -> list(name, () -> new
MonitoringListAdapter<>(name,
+description,
+rowClazz,
+(MonitoringRowAttributeWalker)walkers.get(rowClazz),
+data.get(),
+rowFunc));
+
+//Create new instance of the list.
+listCreator.get();
+
+ctx.metric().addEnableListListener(listenOnlyEqual(name, identity(), n
-> listCreator.get()));
+ctx.metric().addRemoveListListener(l ->
data.get().values().forEach(clearer));
}
-/** {@inheritDoc} */
-@Override public void
addMetricRegistryCreationListener(Consumer lsnr) {
-metricRegCreationLsnrs.add(lsnr);
+/**
+ * @param name Name of the list.
+ * @return List.
+ */
+@Nullable public MonitoringList list(String
name) {
+return (MonitoringList)lists.get(name);
+}
+
+/**
+ * Gets or creates {@link MonitoringList}.
+ *
+ * @param name Name of the list.
+ * @param listSupplier List supplier.
+ * @param Type of the row.
+ * @return Monitoring list.
+ */
+private MonitoringList list(String name,
Review comment:
Let's don't introduce any functional prematurely, follow-up PRs also can
have some fixes after review, so it's not a fact that this method will be
needed.
And here is naming problem again. You have overloaded `registerList`
methods, one of it register some listeners, another one creates the list after
enabling.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine. URL: https://github.com/apache/ignite/pull/6845#discussion_r323591739 ## File path: modules/codegen/src/main/java/org/apache/ignite/codegen/MonitoringRowAttributeWalkerGenerator.java ## @@ -0,0 +1,289 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.codegen; + +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.TreeSet; +import java.util.function.ObjIntConsumer; +import org.apache.ignite.internal.processors.metric.list.walker.Order; +import org.apache.ignite.spi.metric.jmx.MonitoringListMBean; +import org.apache.ignite.spi.metric.list.MonitoringList; +import org.apache.ignite.spi.metric.list.MonitoringRow; +import org.apache.ignite.spi.metric.list.MonitoringRowAttributeWalker; +import org.apache.ignite.spi.metric.list.view.CacheGroupView; +import org.apache.ignite.spi.metric.list.view.CacheView; +import org.apache.ignite.spi.metric.list.view.ComputeTaskView; +import org.apache.ignite.spi.metric.list.view.ServiceView; +import org.apache.ignite.spi.metric.sql.MonitoringListLocalSystemView; + +import static org.apache.ignite.codegen.MessageCodeGenerator.DFLT_SRC_DIR; Review comment: It's not a path for `MonitoringRowAttributeWalkerGenerator`, it's a path of source in the core module. You should use another constant if you want to generate code in another module. But if the path for the core source changes, I think we want to change only one constant. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323592931
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,45 +378,116 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+public void list(String name, String
description,
+Class rowClazz, Supplier> data, Function
rowFunc, Consumer clearer) {
+
+Supplier> listCreator = () -> list(name, () -> new
MonitoringListAdapter<>(name,
+description,
+rowClazz,
+(MonitoringRowAttributeWalker)walkers.get(rowClazz),
+data.get(),
+rowFunc));
+
+//Create new instance of the list.
+listCreator.get();
+
+ctx.metric().addEnableListListener(listenOnlyEqual(name, identity(), n
-> listCreator.get()));
+ctx.metric().addRemoveListListener(l ->
data.get().values().forEach(clearer));
}
-/** {@inheritDoc} */
-@Override public void
addMetricRegistryCreationListener(Consumer lsnr) {
-metricRegCreationLsnrs.add(lsnr);
+/**
+ * @param name Name of the list.
+ * @return List.
+ */
+@Nullable public MonitoringList list(String
name) {
+return (MonitoringList)lists.get(name);
+}
+
+/**
+ * Gets or creates {@link MonitoringList}.
+ *
+ * @param name Name of the list.
+ * @param listSupplier List supplier.
+ * @param Type of the row.
+ * @return Monitoring list.
+ */
+private MonitoringList list(String name,
Review comment:
OK, 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323593329
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,45 +378,116 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+public void list(String name, String
description,
+Class rowClazz, Supplier> data, Function
rowFunc, Consumer clearer) {
Review comment:
In this case, you will have the same problem with the current implementation
too (the only difference is you can reenable the list to work correctly).
But I think it's better to re-register the list if class who owns the data
recreates the map. In this case, the list will always be up to date and no
reenabling is 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323593975
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/list/MonitoringListAdapter.java
##
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.metric.list;
+
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Function;
+import org.apache.ignite.spi.metric.list.MonitoringRow;
+import org.apache.ignite.spi.metric.list.MonitoringRowAttributeWalker;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Monitoring list backed by {@code data} {@link ConcurrentMap}.
+ */
+public class MonitoringListAdapter extends
AbstractMonitoringList {
+/** Data backed by this list. */
+private final ConcurrentMap data;
Review comment:
> And Map say nothing about thread safety.
Thats why I use `ConcurrentMap` which declares:
```
* A {@link java.util.Map} providing thread safety and atomicity
* guarantees.
```
If we use `Collection` we can't force "user" of the list to pass
thread-safety implementation of Collection.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323595410
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,45 +378,116 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+public void list(String name, String
description,
+Class rowClazz, Supplier> data, Function
rowFunc, Consumer clearer) {
Review comment:
OK, 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323615290
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,45 +378,116 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+public void list(String name, String
description,
+Class rowClazz, Supplier> data, Function
rowFunc, Consumer clearer) {
+
+Supplier> listCreator = () -> list(name, () -> new
MonitoringListAdapter<>(name,
+description,
+rowClazz,
+(MonitoringRowAttributeWalker)walkers.get(rowClazz),
+data.get(),
+rowFunc));
+
+//Create new instance of the list.
+listCreator.get();
+
+ctx.metric().addEnableListListener(listenOnlyEqual(name, identity(), n
-> listCreator.get()));
Review comment:
> This will require one more additional listener map because we already have
'List' of the listeners that comes from MonitoringExporterSpi
I think it's fine to have addition list of listeners for
`MonitoringExporterSpi`. So we will have one map of list removers and one list
of listeners.
> I expect very few enable/disable events, and from 10 to 20 lists over
Ignite, so we shouldn't observe performance issues because of name check in
listenOnlyEqual.
It's mostly not a performance issue, but a wrongly used pattern issue (which
leads to readability and undesirability issue).
> Seems, I'm already did it.
Looks much better now!
> What is the issue with the lambdas count, in the first place?
> Do you see some issues with code readability, understandability?
Yes, it's about readability and understandability.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323615290
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,45 +378,116 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+public void list(String name, String
description,
+Class rowClazz, Supplier> data, Function
rowFunc, Consumer clearer) {
+
+Supplier> listCreator = () -> list(name, () -> new
MonitoringListAdapter<>(name,
+description,
+rowClazz,
+(MonitoringRowAttributeWalker)walkers.get(rowClazz),
+data.get(),
+rowFunc));
+
+//Create new instance of the list.
+listCreator.get();
+
+ctx.metric().addEnableListListener(listenOnlyEqual(name, identity(), n
-> listCreator.get()));
Review comment:
> This will require one more additional listener map because we already have
'List' of the listeners that comes from MonitoringExporterSpi
I think it's fine to have addition list of listeners for
`MonitoringExporterSpi`. So we will have one map of list removers and one list
of listeners.
> I expect very few enable/disable events, and from 10 to 20 lists over
Ignite, so we shouldn't observe performance issues because of name check in
listenOnlyEqual.
It's mostly not a performance issue, but a wrongly used pattern issue (which
leads to readability and undesirability issue).
> Seems, I'm already did it.
Looks much better now!
> What is the issue with the lambdas count, in the first place?
> Do you see some issues with code readability, understandability?
Yes, it's about readability and understandability.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323618617
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/list/MonitoringListAdapter.java
##
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.metric.list;
+
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Function;
+import org.apache.ignite.spi.metric.list.MonitoringRow;
+import org.apache.ignite.spi.metric.list.MonitoringRowAttributeWalker;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Monitoring list backed by {@code data} {@link ConcurrentMap}.
+ */
+public class MonitoringListAdapter extends
AbstractMonitoringList {
+/** Data backed by this list. */
+private final ConcurrentMap data;
Review comment:
How you will force a user to pass thread-safety implementation of collection
if there will have another implementation of `MonitoringList` which consumes
`Collection`?
I think it's not a reason to use extended class only to state it's thread
safety.
Also, you can state about thread safety in JavaDoc .
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323628460
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,45 +378,116 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+public void list(String name, String
description,
+Class rowClazz, Supplier> data, Function
rowFunc, Consumer clearer) {
+
+Supplier> listCreator = () -> list(name, () -> new
MonitoringListAdapter<>(name,
+description,
+rowClazz,
+(MonitoringRowAttributeWalker)walkers.get(rowClazz),
+data.get(),
+rowFunc));
+
+//Create new instance of the list.
+listCreator.get();
+
+ctx.metric().addEnableListListener(listenOnlyEqual(name, identity(), n
-> listCreator.get()));
Review comment:
OK, 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323629603
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/list/MonitoringListAdapter.java
##
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.metric.list;
+
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Function;
+import org.apache.ignite.spi.metric.list.MonitoringRow;
+import org.apache.ignite.spi.metric.list.MonitoringRowAttributeWalker;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Monitoring list backed by {@code data} {@link ConcurrentMap}.
+ */
+public class MonitoringListAdapter extends
AbstractMonitoringList {
+/** Data backed by this list. */
+private final ConcurrentMap data;
Review comment:
We don't need a `Collection` backed implementation of monitoring list for
now.
If we need it in the future, we will rework this code.
In the previous comments, you propose do not create code, we don't need
right now.
For now, all we need is an implementation that backed some `ConcurrentMap`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6837: IGNITE-10223: affinity list methods are added, tests are changed
nizhikov commented on a change in pull request #6837: IGNITE-10223: affinity
list methods are added, tests are changed
URL: https://github.com/apache/ignite/pull/6837#discussion_r323631691
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/affinity/GridCacheAffinityProxy.java
##
@@ -221,24 +222,44 @@ public GridCacheAffinityProxy(GridCacheContext
cctx, Affinity delegate)
}
}
-/** {@inheritDoc} */
+/**
+ * {@inheritDoc}
+ *
+ * @deprecated use {@link
GridCacheAffinityProxy#mapKeyToPrimaryAndBackupsList(Object)} instead.
+ */
+@Deprecated
@Override public Collection mapKeyToPrimaryAndBackups(K key) {
+return mapKeyToPrimaryAndBackupsList(key);
+}
+
+/** {@inheritDoc} */
+@Override public List mapKeyToPrimaryAndBackupsList(K key) {
CacheOperationContext old = gate.enter(null);
try {
-return delegate.mapKeyToPrimaryAndBackups(key);
+return delegate.mapKeyToPrimaryAndBackupsList(key);
}
finally {
gate.leave(old);
}
}
-/** {@inheritDoc} */
+/**
+ * {@inheritDoc}
+ *
+ * @deprecated use {@link
GridCacheAffinityProxy#mapKeyToPrimaryAndBackupsList(Object)} instead.
Review comment:
This should start with the capital letter.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6837: IGNITE-10223: affinity list methods are added, tests are changed
nizhikov commented on a change in pull request #6837: IGNITE-10223: affinity
list methods are added, tests are changed
URL: https://github.com/apache/ignite/pull/6837#discussion_r323631606
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/affinity/GridCacheAffinityProxy.java
##
@@ -221,24 +222,44 @@ public GridCacheAffinityProxy(GridCacheContext
cctx, Affinity delegate)
}
}
-/** {@inheritDoc} */
+/**
+ * {@inheritDoc}
+ *
+ * @deprecated use {@link
GridCacheAffinityProxy#mapKeyToPrimaryAndBackupsList(Object)} instead.
Review comment:
This should start with the capital letter.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323631607
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
Review comment:
Why Function and not just a private method in class?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323632105
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+IgniteEx ignite = startGrids(2);
+
+assertEquals(2, ignite.cluster().topologyVersion());
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteNoSsl() throws Exception {
+sslEnabled = false;
+
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+GridTestUtils.assertThrowsAnyCause(log,
+() -> startGrids(2),
+IgniteCheckedException.class,
Review comment:
What is the real cause of not starting ZK Discovery SPI here?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323631399
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+IgniteEx ignite = startGrids(2);
+
+assertEquals(2, ignite.cluster().topologyVersion());
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteNoSsl() throws Exception {
+sslEnabled = false;
+
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+GridTestUtils.assertThrowsAnyCause(log,
+() -> startGrids(2),
+IgniteCheckedException.class,
+"Failed to start SPI: ZookeeperDiscoverySpi");
+}
+
+/**
+ * Setup system properties.
+ */
+private void setupSystemProperties() {
+System.setProperty("zookeeper.serverCnxnFactory",
"org.apache.zookeeper.server.NettyServerCnxnFactory");
+System.setProperty("zookeeper.client.secure", "true");
+System.setProperty("zookeeper.ssl.keyStore.location",
rsrcPath.apply("/server.jks"));
+System.setProperty("zookeeper.ssl.keyStore.password", "123456");
+System.setProperty("zookeeper.ssl.trustStore.location",
rsrcPath.apply("/trust.jks"));
+System.setProperty("zookeeper.ssl.trustStore.password", "123456");
+System.setProperty("zookeeper.ssl.hostnameVerification", "false");
+}
+
+/**
+ * Cleanup system properties.
+ */
+private void clearSystemProperties() {
+System.setProperty("zookeeper.clientCnxnSocket", "");
Review comment:
System.clearProperty can be used instead.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323630904
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiTestBase.java
##
@@ -193,7 +200,12 @@
super.beforeTest();
if (USE_TEST_CLUSTER && zkCluster == null) {
-zkCluster =
ZookeeperDiscoverySpiTestUtil.createTestingCluster(ZK_SRVS);
+Map[] customProps = new Map[ZK_SRVS];
Review comment:
Do base class really need to know about SSL?
I suggest making 2 protected methods:
`clusterCustomProperties` and `connectionString`
that can be overridden and implemented in SSL test.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323630904
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiTestBase.java
##
@@ -193,7 +200,12 @@
super.beforeTest();
if (USE_TEST_CLUSTER && zkCluster == null) {
-zkCluster =
ZookeeperDiscoverySpiTestUtil.createTestingCluster(ZK_SRVS);
+Map[] customProps = new Map[ZK_SRVS];
Review comment:
Do base class really need about SSL?
I suggest making 2 protected methods:
`clusterCustomProperties` and `connectionString`
that can be overridden and implemented in SSL test.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323631792
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
Review comment:
The rule `@WithSystemProperty` can be used instead.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6837: IGNITE-10223: affinity list methods are added, tests are changed
nizhikov commented on a change in pull request #6837: IGNITE-10223: affinity
list methods are added, tests are changed
URL: https://github.com/apache/ignite/pull/6837#discussion_r323633576
##
File path:
modules/core/src/test/java/org/apache/ignite/testframework/junits/multijvm/AffinityProcessProxy.java
##
@@ -102,9 +103,19 @@ public AffinityProcessProxy(String cacheName,
IgniteProcessProxy proxy) {
return compute.call(new MapKeyToNodeTask<>(cacheName, key));
}
-/** {@inheritDoc} */
+/**
+ * {@inheritDoc}
+ *
+ * @deprecated use {@link
AffinityProcessProxy#mapKeyToPrimaryAndBackupsList(Object)} instead.
Review comment:
This should start with the capital letter.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6837: IGNITE-10223: affinity list methods are added, tests are changed
nizhikov commented on a change in pull request #6837: IGNITE-10223: affinity
list methods are added, tests are changed
URL: https://github.com/apache/ignite/pull/6837#discussion_r323633672
##
File path:
modules/core/src/test/java/org/apache/ignite/testframework/junits/multijvm/AffinityProcessProxy.java
##
@@ -117,9 +128,19 @@ public AffinityProcessProxy(String cacheName,
IgniteProcessProxy proxy) {
return compute.call(new MapPartitionsToNodes<>(cacheName, parts));
}
-/** {@inheritDoc} */
+/**
+ * {@inheritDoc}
+ *
+ * @deprecated use {@link
AffinityProcessProxy#mapPartitionToPrimaryAndBackupsList(int)} instead.
Review comment:
This should start with the capital letter.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6837: IGNITE-10223: affinity list methods are added, tests are changed
nizhikov commented on a change in pull request #6837: IGNITE-10223: affinity
list methods are added, tests are changed
URL: https://github.com/apache/ignite/pull/6837#discussion_r323634117
##
File path:
modules/core/src/test/java/org/apache/ignite/testframework/junits/multijvm/AffinityProcessProxy.java
##
@@ -168,22 +189,22 @@ else if (backup)
/**
*
*/
-private static class MapKeyToPrimaryAndBackupsTask extends
AffinityTaskAdapter> {
+private static class mapKeyToPrimaryAndBackupsListTask extends
AffinityTaskAdapter> {
Review comment:
Class name should start with the capital letter.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on issue #6837: IGNITE-10223: affinity list methods are added, tests are changed
nizhikov commented on issue #6837: IGNITE-10223: affinity list methods are added, tests are changed URL: https://github.com/apache/ignite/pull/6837#issuecomment-530737915 Overall, looks good. Thank you! I left some minor comments regarding javadoc and class rename. Please, resolve them prior 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323635809
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -177,9 +222,63 @@
/** Registered metrics registries. */
private final ConcurrentHashMap registries = new
ConcurrentHashMap<>();
+/** Metrics registry. */
+private final ReadOnlyMetricRegistry metricsRegistry = new
ReadOnlyMetricRegistry() {
+/** {@inheritDoc} */
+@NotNull @Override public Iterator iterator() {
+return registries.values().iterator();
+}
+
+/** {@inheritDoc} */
+@Override public void
addMetricRegistryCreationListener(Consumer lsnr) {
+metricRegCreationLsnrs.add(lsnr);
+}
+
+/** {@inheritDoc} */
+@Override public void
addMetricRegistryRemoveListener(Consumer lsnr) {
+metricRegRemoveLsnrs.add(lsnr);
+}
+};
+
+/** Registered lists. */
+private final ConcurrentHashMap> lists = new
ConcurrentHashMap<>();
+
+/** Lists registry. */
+private final ReadOnlyMonitoringListRegistry listRegistry = new
ReadOnlyMonitoringListRegistry() {
+/** {@inheritDoc} */
+@Override public void
addListCreationListener(Consumer> lsnr) {
+listCreationLsnrs.add(lsnr);
+}
+
+/** {@inheritDoc} */
+@Override public void
addListRemoveListener(Consumer> lsnr) {
+listRemoveLsnrs.add(lsnr);
+}
+
+/** {@inheritDoc} */
+@NotNull @Override public Iterator> iterator() {
+return lists.values().iterator();
+}
+};
+
/** Metric registry creation listeners. */
private final List> metricRegCreationLsnrs = new
CopyOnWriteArrayList<>();
+/** Metric registry remove listeners. */
+private final List> metricRegRemoveLsnrs = new
CopyOnWriteArrayList<>();
+
+/** List creation listeners. */
+private final List>> listCreationLsnrs = new
CopyOnWriteArrayList<>();
+
+/** List remove listeners. */
+private final List>> listRemoveLsnrs = new
CopyOnWriteArrayList<>();
+
+/** Internal list remove listeners. */
+private final ConcurrentMap>>
internalRemoveLsnrs = new ConcurrentHashMap<>();
+
+/** List enable listeners. */
+private final ConcurrentMap> listEnableLsnrs =
new ConcurrentHashMap<>();
Review comment:
1. Once again. It's not a listener. Let's rename it to something
like`listCreators`
2. It's not a consumer, there is nothing to consume, you just throw away
parameter which you get in this consumer.
The same for `internalRemoveLsnrs`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on issue #6842: IGNITE-11829: distribute join fix
nizhikov commented on issue #6842: IGNITE-11829: distribute join fix URL: https://github.com/apache/ignite/pull/6842#issuecomment-530739158 LGTM. Thanks, for the contribution! 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323636180
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,47 +379,127 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+/**
+ * Registers list which exports {@link ConcurrentMap} content.
+ *
+ * @param name Name of the list.
+ * @param desc Description of the list.
+ * @param rowCls Row class.
+ * @param data Data of the list.
+ * @param rowFunc value to row function.
+ * @param rowClearer Function that clears data on list removal.
+ * @param List row type.
+ * @param Map data type.
+ */
+public void registerList(String name, String
desc,
+Class rowCls, ConcurrentMap data, Function rowFunc,
Consumer rowClearer) {
+
+Supplier> listCreator = () ->
(MonitoringList)lists.computeIfAbsent(name, n -> {
Review comment:
It's not a supplier, you just throw away returned value. Let's use `Runnable`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov merged pull request #6842: IGNITE-11829: distribute join fix
nizhikov merged pull request #6842: IGNITE-11829: distribute join fix URL: https://github.com/apache/ignite/pull/6842 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323637032
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,47 +379,127 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+/**
+ * Registers list which exports {@link ConcurrentMap} content.
+ *
+ * @param name Name of the list.
+ * @param desc Description of the list.
+ * @param rowCls Row class.
+ * @param data Data of the list.
+ * @param rowFunc value to row function.
+ * @param rowClearer Function that clears data on list removal.
+ * @param List row type.
+ * @param Map data type.
+ */
+public void registerList(String name, String
desc,
+Class rowCls, ConcurrentMap data, Function rowFunc,
Consumer rowClearer) {
+
+Supplier> listCreator = () ->
(MonitoringList)lists.computeIfAbsent(name, n -> {
+MonitoringList list = new MonitoringListAdapter<>(name,
+desc,
+rowCls,
+(MonitoringRowAttributeWalker)walkers.get(rowCls),
+data,
+rowFunc);
+
+notifyListeners(list, listCreationLsnrs, log);
+
+return list;
+});
+
+// Create new instance of the list.
+listCreator.get();
+
+ctx.metric().addEnableListListener(name, n -> listCreator.get());
Review comment:
Let's inline `listCreators.put()` here and pass `listCreators` as `Runnable`
without creating new lambda.
Methods `addEnableListListener` and `addRemoveListListener` can be removed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323635809
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -177,9 +222,63 @@
/** Registered metrics registries. */
private final ConcurrentHashMap registries = new
ConcurrentHashMap<>();
+/** Metrics registry. */
+private final ReadOnlyMetricRegistry metricsRegistry = new
ReadOnlyMetricRegistry() {
+/** {@inheritDoc} */
+@NotNull @Override public Iterator iterator() {
+return registries.values().iterator();
+}
+
+/** {@inheritDoc} */
+@Override public void
addMetricRegistryCreationListener(Consumer lsnr) {
+metricRegCreationLsnrs.add(lsnr);
+}
+
+/** {@inheritDoc} */
+@Override public void
addMetricRegistryRemoveListener(Consumer lsnr) {
+metricRegRemoveLsnrs.add(lsnr);
+}
+};
+
+/** Registered lists. */
+private final ConcurrentHashMap> lists = new
ConcurrentHashMap<>();
+
+/** Lists registry. */
+private final ReadOnlyMonitoringListRegistry listRegistry = new
ReadOnlyMonitoringListRegistry() {
+/** {@inheritDoc} */
+@Override public void
addListCreationListener(Consumer> lsnr) {
+listCreationLsnrs.add(lsnr);
+}
+
+/** {@inheritDoc} */
+@Override public void
addListRemoveListener(Consumer> lsnr) {
+listRemoveLsnrs.add(lsnr);
+}
+
+/** {@inheritDoc} */
+@NotNull @Override public Iterator> iterator() {
+return lists.values().iterator();
+}
+};
+
/** Metric registry creation listeners. */
private final List> metricRegCreationLsnrs = new
CopyOnWriteArrayList<>();
+/** Metric registry remove listeners. */
+private final List> metricRegRemoveLsnrs = new
CopyOnWriteArrayList<>();
+
+/** List creation listeners. */
+private final List>> listCreationLsnrs = new
CopyOnWriteArrayList<>();
+
+/** List remove listeners. */
+private final List>> listRemoveLsnrs = new
CopyOnWriteArrayList<>();
+
+/** Internal list remove listeners. */
+private final ConcurrentMap>>
internalRemoveLsnrs = new ConcurrentHashMap<>();
+
+/** List enable listeners. */
+private final ConcurrentMap> listEnableLsnrs =
new ConcurrentHashMap<>();
Review comment:
1. Once again. It's not a listener. Let's rename it to something
like`listCreators`
2. It's not a consumer, there is nothing to consume, you just throw away
parameter which you get in this consumer. Let's use `Runnable`.
The same for `internalRemoveLsnrs`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323645590
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/list/MonitoringListAdapter.java
##
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.metric.list;
+
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Function;
+import org.apache.ignite.spi.metric.list.MonitoringRow;
+import org.apache.ignite.spi.metric.list.MonitoringRowAttributeWalker;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Monitoring list backed by {@code data} {@link ConcurrentMap}.
+ */
+public class MonitoringListAdapter extends
AbstractMonitoringList {
+/** Data backed by this list. */
+private final ConcurrentMap data;
Review comment:
What I'm seeing is exactly the opposite: we don't need `ConcurrentMap`
backed implementation for now. We need 'Collection' backed implementation and
you use `ConcurrentMap` as a `Collection`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323645590
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/list/MonitoringListAdapter.java
##
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.metric.list;
+
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Function;
+import org.apache.ignite.spi.metric.list.MonitoringRow;
+import org.apache.ignite.spi.metric.list.MonitoringRowAttributeWalker;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Monitoring list backed by {@code data} {@link ConcurrentMap}.
+ */
+public class MonitoringListAdapter extends
AbstractMonitoringList {
+/** Data backed by this list. */
+private final ConcurrentMap data;
Review comment:
What I'm seeing is exactly the opposite: we don't need 'ConcurrentMap'
backed implementation for now. We need 'Collection' backed implementation and
you use `ConcurrentMap` as a `Collection`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323645590
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/list/MonitoringListAdapter.java
##
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.metric.list;
+
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Function;
+import org.apache.ignite.spi.metric.list.MonitoringRow;
+import org.apache.ignite.spi.metric.list.MonitoringRowAttributeWalker;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Monitoring list backed by {@code data} {@link ConcurrentMap}.
+ */
+public class MonitoringListAdapter extends
AbstractMonitoringList {
+/** Data backed by this list. */
+private final ConcurrentMap data;
Review comment:
What I'm seeing is exactly the opposite: we don't need `ConcurrentMap`
backed implementation for now. We need `Collection` backed implementation
(inside the engine) and you use `ConcurrentMap` as a `Collection`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] denis-chudov opened a new pull request #6864: GG-23835 Fix negative time in Transaction time dump
denis-chudov opened a new pull request #6864: GG-23835 Fix negative time in Transaction time dump URL: https://github.com/apache/ignite/pull/6864 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] NSAmelchev commented on a change in pull request #6738: IGNITE-12027 NPE on try to read the MinimumNumberOfPartitionCopies metric.
NSAmelchev commented on a change in pull request #6738: IGNITE-12027 NPE on try
to read the MinimumNumberOfPartitionCopies metric.
URL: https://github.com/apache/ignite/pull/6738#discussion_r323675359
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheGroupMetricsImpl.java
##
@@ -444,11 +472,25 @@ public void remove() {
* @return Database.
*/
private GridCacheDatabaseSharedManager database() {
+if (!(ctx.shared().database() instanceof
GridCacheDatabaseSharedManager))
+return null;
+
return (GridCacheDatabaseSharedManager)ctx.shared().database();
}
/** @return Metric group name. */
private String metricGroupName() {
return metricName(CACHE_GROUP_METRICS_PREFIX, ctx.cacheOrGroupName());
}
+
+/** @return {@code True} if topology started. */
+private boolean isTopologyStarted() {
Review comment:
Done. TC bot is OK. @nizhikov Take a look, 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323676178
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+IgniteEx ignite = startGrids(2);
+
+assertEquals(2, ignite.cluster().topologyVersion());
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteNoSsl() throws Exception {
+sslEnabled = false;
+
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+GridTestUtils.assertThrowsAnyCause(log,
+() -> startGrids(2),
+IgniteCheckedException.class,
Review comment:
`KeeperException.SessionExpiredException` with message "KeeperErrorCode =
Session expired for /apacheIgnite". But I used Ignite exception to have no
dependency on Zookeeper here.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323676402
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/service/IgniteServiceProcessor.java
##
@@ -185,6 +188,12 @@
*/
public IgniteServiceProcessor(GridKernalContext ctx) {
super(ctx);
+
+ctx.metric().registerList(SVCS_MON_LIST, SVCS_MON_LIST_DESC,
+ServiceView.class,
+registeredServices,
+s -> s,
+s -> {});
Review comment:
If there are plans to use more such no-op cleaners in following-up PRs,
perhaps it's better to pass `null` and correctly handle nulls in `registerList`
method.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323677969
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
Review comment:
`@WithSystemProperty(key = "zookeeper.clientCnxnSocket", value =
"org.apache.zookeeper.ClientCnxnSocketNetty")` doesn't work.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323677969
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
Review comment:
`@WithSystemProperty(key = "zookeeper.clientCnxnSocket", value =
"org.apache.zookeeper.ClientCnxnSocketNetty")` doesn't work. Ignite can't
connect to ZK.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323678265
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -177,9 +222,63 @@
/** Registered metrics registries. */
private final ConcurrentHashMap registries = new
ConcurrentHashMap<>();
+/** Metrics registry. */
+private final ReadOnlyMetricRegistry metricsRegistry = new
ReadOnlyMetricRegistry() {
+/** {@inheritDoc} */
+@NotNull @Override public Iterator iterator() {
+return registries.values().iterator();
+}
+
+/** {@inheritDoc} */
+@Override public void
addMetricRegistryCreationListener(Consumer lsnr) {
+metricRegCreationLsnrs.add(lsnr);
+}
+
+/** {@inheritDoc} */
+@Override public void
addMetricRegistryRemoveListener(Consumer lsnr) {
+metricRegRemoveLsnrs.add(lsnr);
+}
+};
+
+/** Registered lists. */
+private final ConcurrentHashMap> lists = new
ConcurrentHashMap<>();
+
+/** Lists registry. */
+private final ReadOnlyMonitoringListRegistry listRegistry = new
ReadOnlyMonitoringListRegistry() {
+/** {@inheritDoc} */
+@Override public void
addListCreationListener(Consumer> lsnr) {
+listCreationLsnrs.add(lsnr);
+}
+
+/** {@inheritDoc} */
+@Override public void
addListRemoveListener(Consumer> lsnr) {
+listRemoveLsnrs.add(lsnr);
+}
+
+/** {@inheritDoc} */
+@NotNull @Override public Iterator> iterator() {
+return lists.values().iterator();
+}
+};
+
/** Metric registry creation listeners. */
private final List> metricRegCreationLsnrs = new
CopyOnWriteArrayList<>();
+/** Metric registry remove listeners. */
+private final List> metricRegRemoveLsnrs = new
CopyOnWriteArrayList<>();
+
+/** List creation listeners. */
+private final List>> listCreationLsnrs = new
CopyOnWriteArrayList<>();
+
+/** List remove listeners. */
+private final List>> listRemoveLsnrs = new
CopyOnWriteArrayList<>();
+
+/** Internal list remove listeners. */
+private final ConcurrentMap>>
internalRemoveLsnrs = new ConcurrentHashMap<>();
+
+/** List enable listeners. */
+private final ConcurrentMap> listEnableLsnrs =
new ConcurrentHashMap<>();
Review comment:
Renamed to `task`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323677969
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
Review comment:
`@WithSystemProperty(key = "zookeeper.clientCnxnSocket", value =
"org.apache.zookeeper.ClientCnxnSocketNetty")` doesn't work. Ignite trying to
connect without SSL.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323678802
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,47 +379,127 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+/**
+ * Registers list which exports {@link ConcurrentMap} content.
+ *
+ * @param name Name of the list.
+ * @param desc Description of the list.
+ * @param rowCls Row class.
+ * @param data Data of the list.
+ * @param rowFunc value to row function.
+ * @param rowClearer Function that clears data on list removal.
+ * @param List row type.
+ * @param Map data type.
+ */
+public void registerList(String name, String
desc,
+Class rowCls, ConcurrentMap data, Function rowFunc,
Consumer rowClearer) {
+
+Supplier> listCreator = () ->
(MonitoringList)lists.computeIfAbsent(name, n -> {
+MonitoringList list = new MonitoringListAdapter<>(name,
+desc,
+rowCls,
+(MonitoringRowAttributeWalker)walkers.get(rowCls),
+data,
+rowFunc);
+
+notifyListeners(list, listCreationLsnrs, log);
+
+return list;
+});
+
+// Create new instance of the list.
+listCreator.get();
+
+ctx.metric().addEnableListListener(name, n -> listCreator.get());
Review comment:
Fixed.
> Methods addEnableListListener and addRemoveListListener can be removed.
But these methods still required.
Can you clarify, how it can be removed?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323677969
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
Review comment:
`@WithSystemProperty(key = "zookeeper.clientCnxnSocket", value =
"org.apache.zookeeper.ClientCnxnSocketNetty")` doesn't work.
I got error `java.lang.AssertionError: Failed to wait for Zookeeper testing
cluster ready.`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323686010
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
##
@@ -265,47 +379,127 @@ public MetricRegistry registry(String name) {
return registries.computeIfAbsent(name, n -> {
MetricRegistry mreg = new MetricRegistry(name, log);
-notifyListeners(mreg, metricRegCreationLsnrs);
+notifyListeners(mreg, metricRegCreationLsnrs, log);
return mreg;
});
}
-/** {@inheritDoc} */
-@NotNull @Override public Iterator iterator() {
-return registries.values().iterator();
+/**
+ * Registers list which exports {@link ConcurrentMap} content.
+ *
+ * @param name Name of the list.
+ * @param desc Description of the list.
+ * @param rowCls Row class.
+ * @param data Data of the list.
+ * @param rowFunc value to row function.
+ * @param rowClearer Function that clears data on list removal.
+ * @param List row type.
+ * @param Map data type.
+ */
+public void registerList(String name, String
desc,
+Class rowCls, ConcurrentMap data, Function rowFunc,
Consumer rowClearer) {
+
+Supplier> listCreator = () ->
(MonitoringList)lists.computeIfAbsent(name, n -> {
+MonitoringList list = new MonitoringListAdapter<>(name,
+desc,
+rowCls,
+(MonitoringRowAttributeWalker)walkers.get(rowCls),
+data,
+rowFunc);
+
+notifyListeners(list, listCreationLsnrs, log);
+
+return list;
+});
+
+// Create new instance of the list.
+listCreator.get();
+
+ctx.metric().addEnableListListener(name, n -> listCreator.get());
Review comment:
These methods used only once, they are one-liners, can be inlined and
removed, code will be simplified.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323687261
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiTestBase.java
##
@@ -193,7 +200,12 @@
super.beforeTest();
if (USE_TEST_CLUSTER && zkCluster == null) {
-zkCluster =
ZookeeperDiscoverySpiTestUtil.createTestingCluster(ZK_SRVS);
+Map[] customProps = new Map[ZK_SRVS];
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323687306
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+IgniteEx ignite = startGrids(2);
+
+assertEquals(2, ignite.cluster().topologyVersion());
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteNoSsl() throws Exception {
+sslEnabled = false;
+
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+GridTestUtils.assertThrowsAnyCause(log,
+() -> startGrids(2),
+IgniteCheckedException.class,
+"Failed to start SPI: ZookeeperDiscoverySpi");
+}
+
+/**
+ * Setup system properties.
+ */
+private void setupSystemProperties() {
+System.setProperty("zookeeper.serverCnxnFactory",
"org.apache.zookeeper.server.NettyServerCnxnFactory");
+System.setProperty("zookeeper.client.secure", "true");
+System.setProperty("zookeeper.ssl.keyStore.location",
rsrcPath.apply("/server.jks"));
+System.setProperty("zookeeper.ssl.keyStore.password", "123456");
+System.setProperty("zookeeper.ssl.trustStore.location",
rsrcPath.apply("/trust.jks"));
+System.setProperty("zookeeper.ssl.trustStore.password", "123456");
+System.setProperty("zookeeper.ssl.hostnameVerification", "false");
+}
+
+/**
+ * Cleanup system properties.
+ */
+private void clearSystemProperties() {
+System.setProperty("zookeeper.clientCnxnSocket", "");
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323687494
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323689188
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java
##
@@ -7040,6 +7042,51 @@ public static String toShortString(Collection ns) {
return sb.a(']').toString();
}
+/**
+ * Get string representation of an object properly catching all exceptions.
+ *
+ * @param obj Object.
+ * @return Result or {@code null}.
+ */
+@Nullable public static String toStringSafe(@Nullable Object obj) {
+if (obj == null)
+return null;
+else {
+try {
+return obj.toString();
+}
+catch (Exception e) {
+try {
+return "Failed to convert object to string: " +
e.getMessage();
+}
+catch (Exception e0) {
+return "Failed to convert object to string (error message
is not available)";
+}
+}
+}
+}
+
+/**
+ * @param objs Collection of objects.
+ * @return String representation.
+ */
+@Nullable public static String toStringSafe(@Nullable Collection
objs) {
Review comment:
See no usages of this method. Did I miss something?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323689797
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java
##
@@ -7040,6 +7042,51 @@ public static String toShortString(Collection ns) {
return sb.a(']').toString();
}
+/**
+ * Get string representation of an object properly catching all exceptions.
+ *
+ * @param obj Object.
+ * @return Result or {@code null}.
+ */
+@Nullable public static String toStringSafe(@Nullable Object obj) {
+if (obj == null)
+return null;
+else {
+try {
+return obj.toString();
+}
+catch (Exception e) {
+try {
+return "Failed to convert object to string: " +
e.getMessage();
+}
+catch (Exception e0) {
+return "Failed to convert object to string (error message
is not available)";
+}
+}
+}
+}
+
+/**
+ * @param objs Collection of objects.
+ * @return String representation.
+ */
+@Nullable public static String toStringSafe(@Nullable Collection
objs) {
Review comment:
`CacheGroupView#affinity`
`CacheView#topologyValidator`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323689797
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java
##
@@ -7040,6 +7042,51 @@ public static String toShortString(Collection ns) {
return sb.a(']').toString();
}
+/**
+ * Get string representation of an object properly catching all exceptions.
+ *
+ * @param obj Object.
+ * @return Result or {@code null}.
+ */
+@Nullable public static String toStringSafe(@Nullable Object obj) {
+if (obj == null)
+return null;
+else {
+try {
+return obj.toString();
+}
+catch (Exception e) {
+try {
+return "Failed to convert object to string: " +
e.getMessage();
+}
+catch (Exception e0) {
+return "Failed to convert object to string (error message
is not available)";
+}
+}
+}
+}
+
+/**
+ * @param objs Collection of objects.
+ * @return String representation.
+ */
+@Nullable public static String toStringSafe(@Nullable Collection
objs) {
Review comment:
`CacheGroupView#affinity`
`CacheView#topologyValidator`
Collection case will be use in the next PRs, removed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323705639
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
Review comment:
Looks like annotation processed before zk cluster is up, and it breaks
something inside zk nodes.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323712546
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiTestBase.java
##
@@ -193,7 +200,12 @@
super.beforeTest();
if (USE_TEST_CLUSTER && zkCluster == null) {
-zkCluster =
ZookeeperDiscoverySpiTestUtil.createTestingCluster(ZK_SRVS);
+Map[] customProps = new Map[ZK_SRVS];
Review comment:
Still see SSL leftovers in base class - `ZK_SECURE_CLIENT_PORT` and
`getSslConnectString`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323714398
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTestBase.java
##
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.spi.discovery.zk.ZookeeperDiscoverySpi;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+class ZookeeperDiscoverySpiSslTestBase extends ZookeeperDiscoverySpiTestBase {
+/** */
+protected boolean sslEnabled;
+
+/** {@inheritDoc} */
+@Override protected Map[] clusterCustomProperties() {
+Map[] customProps = super.clusterCustomProperties();
+
+for (int i = 0; i < ZK_SRVS; i++) {
+Map props = customProps[i];
+
+if (props == null)
+props = new HashMap<>();
+
+props.put(ZK_SECURE_CLIENT_PORT, String.valueOf(2281 + i));
+
+customProps[i] = props;
+}
+
+return customProps;
+}
+
+/** {@inheritDoc} */
+@Override protected void setupZkConnectionString(ZookeeperDiscoverySpi
zkSpi) {
Review comment:
I think you shouldn't copy-paste whole logic regarding to setting up ZK
connection string.
You should abstract the only place returning connection string setting to
zkSpi.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323715442
##
File path:
modules/core/src/main/java/org/apache/ignite/spi/metric/list/MonitoringRow.java
##
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.metric.list;
+
+import org.apache.ignite.internal.processors.metric.GridMetricManager;
+import org.apache.ignite.spi.metric.ReadOnlyMonitoringListRegistry;
+
+/**
+ * Monitoring list row base interface.
+ * Each row idenitified by the instance of {@code Id}.
Review comment:
I think we should describe explicitly in JavaDoc which types of fields are
acceptable for MonitoringRow view.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323720346
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiTestBase.java
##
@@ -193,7 +200,12 @@
super.beforeTest();
if (USE_TEST_CLUSTER && zkCluster == null) {
-zkCluster =
ZookeeperDiscoverySpiTestUtil.createTestingCluster(ZK_SRVS);
+Map[] customProps = new Map[ZK_SRVS];
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323720189
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTestBase.java
##
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.spi.discovery.zk.ZookeeperDiscoverySpi;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+class ZookeeperDiscoverySpiSslTestBase extends ZookeeperDiscoverySpiTestBase {
+/** */
+protected boolean sslEnabled;
+
+/** {@inheritDoc} */
+@Override protected Map[] clusterCustomProperties() {
+Map[] customProps = super.clusterCustomProperties();
+
+for (int i = 0; i < ZK_SRVS; i++) {
+Map props = customProps[i];
+
+if (props == null)
+props = new HashMap<>();
+
+props.put(ZK_SECURE_CLIENT_PORT, String.valueOf(2281 + i));
+
+customProps[i] = props;
+}
+
+return customProps;
+}
+
+/** {@inheritDoc} */
+@Override protected void setupZkConnectionString(ZookeeperDiscoverySpi
zkSpi) {
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323727241
##
File path:
modules/core/src/main/java/org/apache/ignite/spi/metric/list/MonitoringRow.java
##
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.metric.list;
+
+import org.apache.ignite.internal.processors.metric.GridMetricManager;
+import org.apache.ignite.spi.metric.ReadOnlyMonitoringListRegistry;
+
+/**
+ * Monitoring list row base interface.
+ * Each row idenitified by the instance of {@code Id}.
Review comment:
"Each row idenitified by the instance of {@code Id}" what does it mean? I
didn't see any "Id" field in *View classes.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323728128
##
File path:
modules/core/src/main/java/org/apache/ignite/spi/metric/list/MonitoringRow.java
##
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.metric.list;
+
+import org.apache.ignite.internal.processors.metric.GridMetricManager;
+import org.apache.ignite.spi.metric.ReadOnlyMonitoringListRegistry;
+
+/**
+ * Monitoring list row base interface.
+ * Each row idenitified by the instance of {@code Id}.
Review comment:
It mean "obsolete" comment :). Removed
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323729706
##
File path:
modules/core/src/main/java/org/apache/ignite/spi/metric/list/MonitoringRowAttributeWalker.java
##
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.metric.list;
+
+/**
+ * Utility class for quick iteration over {@link MonitoringRow} properties.
+ */
+public interface MonitoringRowAttributeWalker {
+/** @return Count of a row properties. */
+public int count();
+
+/**
+ * Calls visitor for each {@link MonitoringRow} attribute.
+ *
+ * @param visitor Attribute visitor.
+ */
+public void visitAll(AttributeVisitor visitor);
+
+/**
+ * Calls visitor for each {@link MonitoringRow} attribute.
+ * Value of the attribute also provided.
+ *
+ * @param row Row to iterate.
+ * @param visitor Attribute visitor.
+ */
+public void visitAll(R row, AttributeWithValueVisitor visitor);
+
+/** Attribute visitor. */
+public interface AttributeVisitor {
+/**
+ * Visit some object property.
+ * @param idx Index.
+ * @param name Name.
+ * @param clazz Value class.
+ * @param Value type.
+ */
+public void accept(int idx, String name, Class clazz);
+
+/**
+ * Visit attribute. Attribute value is {@code boolean} primitive.
+ *
+ * @param idx Index.
+ * @param name Name.
+ */
+public void acceptBoolean(int idx, String name);
Review comment:
Do we really need all these `accept*` methods? Can we reuse `accept(idx,
name, clazz)` method?
In `AttributeWithValueVisitor` this methods helps to avoid boxing, but in
`AttributeVisitor` no value passed to methods, so reuse of `accept()` will
simplify the 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
nizhikov commented on a change in pull request #6845: IGNITE-12145: Monitoring
list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323730801
##
File path:
modules/core/src/main/java/org/apache/ignite/spi/metric/list/MonitoringRowAttributeWalker.java
##
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.metric.list;
+
+/**
+ * Utility class for quick iteration over {@link MonitoringRow} properties.
+ */
+public interface MonitoringRowAttributeWalker {
+/** @return Count of a row properties. */
+public int count();
+
+/**
+ * Calls visitor for each {@link MonitoringRow} attribute.
+ *
+ * @param visitor Attribute visitor.
+ */
+public void visitAll(AttributeVisitor visitor);
+
+/**
+ * Calls visitor for each {@link MonitoringRow} attribute.
+ * Value of the attribute also provided.
+ *
+ * @param row Row to iterate.
+ * @param visitor Attribute visitor.
+ */
+public void visitAll(R row, AttributeWithValueVisitor visitor);
+
+/** Attribute visitor. */
+public interface AttributeVisitor {
+/**
+ * Visit some object property.
+ * @param idx Index.
+ * @param name Name.
+ * @param clazz Value class.
+ * @param Value type.
+ */
+public void accept(int idx, String name, Class clazz);
+
+/**
+ * Visit attribute. Attribute value is {@code boolean} primitive.
+ *
+ * @param idx Index.
+ * @param name Name.
+ */
+public void acceptBoolean(int idx, String name);
Review comment:
Seems, we simplify and make code more quick with the accept* for primitives.
Isn't 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323734169
##
File path:
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/CacheComparatorTest.java
##
@@ -42,10 +42,8 @@ public void testDirect() {
null, true, null, true,
false, null, new QuerySchema(), null);
-assertEquals(-1,
-ClusterCachesInfo.CacheComparators.DIRECT.compare(desc1, desc2));
+assertTrue(ClusterCachesInfo.CacheComparators.DIRECT.compare(desc1,
desc2) < 0);
Review comment:
After `compare` method was fixed this change is redundant.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #6845: IGNITE-12145: Monitoring list engine.
alex-plekhanov commented on a change in pull request #6845: IGNITE-12145:
Monitoring list engine.
URL: https://github.com/apache/ignite/pull/6845#discussion_r323750481
##
File path:
modules/core/src/main/java/org/apache/ignite/spi/metric/list/MonitoringRowAttributeWalker.java
##
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.metric.list;
+
+/**
+ * Utility class for quick iteration over {@link MonitoringRow} properties.
+ */
+public interface MonitoringRowAttributeWalker {
+/** @return Count of a row properties. */
+public int count();
+
+/**
+ * Calls visitor for each {@link MonitoringRow} attribute.
+ *
+ * @param visitor Attribute visitor.
+ */
+public void visitAll(AttributeVisitor visitor);
+
+/**
+ * Calls visitor for each {@link MonitoringRow} attribute.
+ * Value of the attribute also provided.
+ *
+ * @param row Row to iterate.
+ * @param visitor Attribute visitor.
+ */
+public void visitAll(R row, AttributeWithValueVisitor visitor);
+
+/** Attribute visitor. */
+public interface AttributeVisitor {
+/**
+ * Visit some object property.
+ * @param idx Index.
+ * @param name Name.
+ * @param clazz Value class.
+ * @param Value type.
+ */
+public void accept(int idx, String name, Class clazz);
+
+/**
+ * Visit attribute. Attribute value is {@code boolean} primitive.
+ *
+ * @param idx Index.
+ * @param name Name.
+ */
+public void acceptBoolean(int idx, String name);
Review comment:
How we simplify the code with several accept* methods? I see only
duplication in 3 exporters implementations. And not sure about more quick code.
We only saving some `if` branch checks for list headers generation. I think
it's absolutely nothing.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323765542
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+IgniteEx ignite = startGrids(2);
+
+assertEquals(2, ignite.cluster().topologyVersion());
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteNoSsl() throws Exception {
+sslEnabled = false;
+
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+GridTestUtils.assertThrowsAnyCause(log,
+() -> startGrids(2),
+IgniteCheckedException.class,
Review comment:
Please correct me if I'm not right.
When we setup ZK Client SSL socket and trying to connect to a ZK cluster
which doesn't support SSL, we successfully establish a connection to the
cluster but after some time ZK client session is expired? Or connection is not
established and we just wait for some timeout and breaking by this message?
It looks suspicious to me. For users that trying to use SSL, this message
will not say anything useful how to fix such a situation. Can we somehow
intercept this exception in the ZK client and we have set SSL socket property
to tell to the user a hint, that maybe his ZK cluster is not secure?
Could you please also debug deeper this case? Maybe we get a somewhere
correct exception during the connecting process and just not pay attention on
that?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ibessonov commented on a change in pull request #6814: IGNITE-12108 TCP Communication Metrics ported to a new framework.
ibessonov commented on a change in pull request #6814: IGNITE-12108 TCP
Communication Metrics ported to a new framework.
URL: https://github.com/apache/ignite/pull/6814#discussion_r323777536
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/util/nio/GridNioServer.java
##
@@ -2860,14 +2895,17 @@ final void reset0() {
* Gets outbound messages queue size.
*
* @return Write queue size.
+ * @deprecated Will be removed in the next major release and replaced with
new metrics API.
*/
+@Deprecated
public int outboundMessagesQueueSize() {
-int res = 0;
-
-for (GridSelectorNioSessionImpl ses : sessions)
-res += ses.writeQueueSize();
+if (mreg == null)
+return -1;
-return res;
+return (int) mreg.longAdderMetric(
Review comment:
I don't get it, can you please explain this comment in more details? Is this
about excessive memory allocation?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323801188
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
Review comment:
Okay, let's left it as 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
SomeFire commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323801504
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+IgniteEx ignite = startGrids(2);
+
+assertEquals(2, ignite.cluster().topologyVersion());
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteNoSsl() throws Exception {
+sslEnabled = false;
+
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+GridTestUtils.assertThrowsAnyCause(log,
+() -> startGrids(2),
+IgniteCheckedException.class,
Review comment:
Connection is not established and we just wait for some timeout. Client is
trying to connect again and again until timeout, but server closes connection
every time because of wrong message length.
Currently we write message to Ignite log:
```
[ZookeeperClient] Operation failed with unexpected error, connection lost:
org.apache.zookeeper.KeeperException$SessionExpiredException: KeeperErrorCode =
Session expired for /apacheIgnite
```
I can change message to
```
[ZookeeperClient] Operation failed with unexpected error, connection lost.
Check connection configuration.
[err=org.apache.zookeeper.KeeperException$SessionExpiredException:
KeeperErrorCode = Session expired for /apacheIgnite]
```
Or add "check connection" inside the exception.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323839169
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+IgniteEx ignite = startGrids(2);
+
+assertEquals(2, ignite.cluster().topologyVersion());
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteNoSsl() throws Exception {
+sslEnabled = false;
+
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+GridTestUtils.assertThrowsAnyCause(log,
+() -> startGrids(2),
+IgniteCheckedException.class,
Review comment:
What if we use both SSL on client and server, but a client has incompatible
with server nodes keyStore/trustStore? Will it result in the same behavior? I
think we need to add a test for that also. Try to up a client in separate JVM
(It can be done overriding method `isMultiJvm` to `true` and adding client
system properties overriding method `additionalRemoteJvmArgs`)
I think we need to propagate in exception a message that connection was not
established and also if system property indicates that SSL is enabled exists we
need to add to the exception a hint to check SSL configuration.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323839169
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+IgniteEx ignite = startGrids(2);
+
+assertEquals(2, ignite.cluster().topologyVersion());
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteNoSsl() throws Exception {
+sslEnabled = false;
+
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+GridTestUtils.assertThrowsAnyCause(log,
+() -> startGrids(2),
+IgniteCheckedException.class,
Review comment:
What if we use both SSL on client and server, but a client has incompatible
with server nodes keyStore/trustStore? Will it result in the same behavior? I
think we need to add a test for that also. Try to up a node in separate JVM (It
can be done overriding method `isMultiJvm` to `true` and adding zk secure
client system properties overriding method `additionalRemoteJvmArgs`)
I think we need to propagate in exception a message that connection was not
established and also if system property indicates that SSL is enabled exists we
need to add to the exception a hint to check SSL configuration.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL support for ignite zookeeper SPI
Jokser commented on a change in pull request #6799: IGNITE-11094 Add SSL
support for ignite zookeeper SPI
URL: https://github.com/apache/ignite/pull/6799#discussion_r323839169
##
File path:
modules/zookeeper/src/test/java/org/apache/ignite/spi/discovery/zk/internal/ZookeeperDiscoverySpiSslTest.java
##
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.zk.internal;
+
+import java.nio.file.Paths;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Base class for Zookeeper SPI discovery tests in this package. It is
intended to provide common overrides for
+ * superclass methods to be shared by all subclasses.
+ */
+public class ZookeeperDiscoverySpiSslTest extends
ZookeeperDiscoverySpiTestBase {
+/** Ignite home. */
+private static final String IGNITE_HOME = U.getIgniteHome();
+
+/** Resource path. */
+private static final Function rsrcPath = rsrc -> Paths.get(
+IGNITE_HOME == null ? "." : IGNITE_HOME,
+"modules",
+"core",
+"src",
+"test",
+"resources",
+rsrc
+).toString();
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+sslEnabled = true;
+
+setupSystemProperties();
+
+super.beforeTest();
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteSsl() throws Exception {
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+IgniteEx ignite = startGrids(2);
+
+assertEquals(2, ignite.cluster().topologyVersion());
+}
+
+/**
+ * @throws Exception if failed.
+ */
+@Test
+public void testIgniteNoSsl() throws Exception {
+sslEnabled = false;
+
+System.setProperty("zookeeper.clientCnxnSocket",
"org.apache.zookeeper.ClientCnxnSocketNetty");
+
+GridTestUtils.assertThrowsAnyCause(log,
+() -> startGrids(2),
+IgniteCheckedException.class,
Review comment:
What if we use both SSL on client and server, but a client has incompatible
with server nodes keyStore/trustStore? Will it result in the same behavior? I
think we need to add a test for that also. Try to up a node in separate JVM (It
can be done overriding method `isMultiJvm` to `true` and adding client system
properties overriding method `additionalRemoteJvmArgs`)
I think we need to propagate in exception a message that connection was not
established and also if system property indicates that SSL is enabled exists we
need to add to the exception a hint to check SSL configuration.
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:
us...@infra.apache.org
With regards,
Apache Git Services
