Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
gerlowskija commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1709424728
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/icons/SolrLogo.kt:
##
@@ -0,0 +1,257 @@
+/*
+ * 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.solr.composeui.ui.icons
+
+import androidx.compose.foundation.layout.Box
+import androidx.compose.material.icons.materialPath
+import androidx.compose.material3.Icon
+import androidx.compose.material3.MaterialTheme
+import androidx.compose.runtime.Composable
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.graphics.vector.ImageVector
+import androidx.compose.ui.graphics.vector.rememberVectorPainter
+import androidx.compose.ui.unit.dp
+import org.apache.solr.compose_ui.generated.resources.Res
+import org.apache.solr.compose_ui.generated.resources.cd_solr_logo
+import org.jetbrains.compose.resources.stringResource
+
+/**
+ * Colorized Solr logo.
+ *
+ * @param modifier Modifier that is applied to the wrapper of the logo.
+ */
+@Composable
+fun SolrLogo(
+modifier: Modifier = Modifier,
+) {
+val solrPainter = rememberVectorPainter(
+ImageVector.Builder(
+name = "Logos.SolrLogo",
+defaultWidth = 128.dp,
+defaultHeight = 64.dp,
+viewportWidth = 128f,
+viewportHeight = 64f,
+autoMirror = false,
+).apply {
+materialPath {
+moveTo(25.7013f, 44.178f)
Review Comment:
Oh ok, wow. I never would've guessed that.
Or, I guess I knew about wanting the logo to scale crisply, but I figured
that'd involve using an svg image file as an asset, rather than writing code to
draw points directly.
But makes sense!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
gerlowskija commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1709425822
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/main/MainContent.kt:
##
@@ -0,0 +1,78 @@
+/*
+ * 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.solr.composeui.ui.main
+
+import androidx.compose.foundation.layout.Row
+import androidx.compose.foundation.layout.fillMaxHeight
+import androidx.compose.foundation.layout.fillMaxWidth
+import androidx.compose.foundation.layout.padding
+import androidx.compose.foundation.layout.width
+import androidx.compose.foundation.rememberScrollState
+import androidx.compose.foundation.verticalScroll
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.getValue
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.unit.dp
+import com.arkivanov.decompose.extensions.compose.stack.Children
+import com.arkivanov.decompose.extensions.compose.subscribeAsState
+import org.apache.solr.composeui.components.main.MainComponent
+import org.apache.solr.composeui.components.main.integration.asMainMenu
+import org.apache.solr.composeui.ui.environment.EnvironmentContent
+import org.apache.solr.composeui.ui.logging.LoggingContent
+import org.apache.solr.composeui.ui.navigation.NavigationSideBar
+
+/**
+ * The composable used for users that have already authenticated.
+ *
+ * @param component Component that manages the state of the composable.
+ */
+@Composable
+fun MainContent(
+component: MainComponent,
+modifier: Modifier = Modifier,
+) {
+val childStack by component.childStack.subscribeAsState()
+val scrollState = rememberScrollState()
+
+Row(modifier = modifier) {
+NavigationSideBar(
+modifier = Modifier.fillMaxHeight()
+.width(224.dp),
+selectedItem = childStack.active.instance.asMainMenu,
+onNavigate = component::onNavigate,
+)
+
+Children(
+stack = component.childStack,
+modifier = Modifier.weight(1f),
+) {
+when(val child = it.instance) {
+is MainComponent.Child.Environment -> EnvironmentContent(
+component = child.component,
+modifier = Modifier.fillMaxWidth()
+.verticalScroll(scrollState)
+.padding(16.dp),
Review Comment:
Perfect, I've got some reading to do, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
gerlowskija commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1709424728
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/icons/SolrLogo.kt:
##
@@ -0,0 +1,257 @@
+/*
+ * 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.solr.composeui.ui.icons
+
+import androidx.compose.foundation.layout.Box
+import androidx.compose.material.icons.materialPath
+import androidx.compose.material3.Icon
+import androidx.compose.material3.MaterialTheme
+import androidx.compose.runtime.Composable
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.graphics.vector.ImageVector
+import androidx.compose.ui.graphics.vector.rememberVectorPainter
+import androidx.compose.ui.unit.dp
+import org.apache.solr.compose_ui.generated.resources.Res
+import org.apache.solr.compose_ui.generated.resources.cd_solr_logo
+import org.jetbrains.compose.resources.stringResource
+
+/**
+ * Colorized Solr logo.
+ *
+ * @param modifier Modifier that is applied to the wrapper of the logo.
+ */
+@Composable
+fun SolrLogo(
+modifier: Modifier = Modifier,
+) {
+val solrPainter = rememberVectorPainter(
+ImageVector.Builder(
+name = "Logos.SolrLogo",
+defaultWidth = 128.dp,
+defaultHeight = 64.dp,
+viewportWidth = 128f,
+viewportHeight = 64f,
+autoMirror = false,
+).apply {
+materialPath {
+moveTo(25.7013f, 44.178f)
Review Comment:
Oh ok, wow. I never would've guessed that, but it makes sense I guess.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
gerlowskija commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1709410616 ## dev-docs/ui/technology-overview.adoc: ## @@ -0,0 +1,83 @@ += Technology Overview Review Comment: Yeah, it ended up not being too bad once I got into the actual PR code. The docs set great context though -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17394: Check response status in IndexFetcher [solr]
gerlowskija merged PR #2621: URL: https://github.com/apache/solr/pull/2621 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17391: Cached thread pools actually have more than one thread. [solr]
psalagnac commented on PR #2619: URL: https://github.com/apache/solr/pull/2619#issuecomment-2275386537 I also updated Javadoc of the other variant (unbounded pool) to reduce confusion. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17391: Cached thread pools actually have more than one thread. [solr]
psalagnac commented on PR #2619: URL: https://github.com/apache/solr/pull/2619#issuecomment-2275296108 > Are we good with the CHANGES.txt and other changes? (Pierre or anyone else?) I would also mention shard Split, as it was also impacted. A single thread per node was processing all backup/restore/split command. All other changes are good to me. Thanks for handling this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17359: Move Zk Arg parsing into Java Code [solr]
rahulgoswami commented on PR #2593: URL: https://github.com/apache/solr/pull/2593#issuecomment-2275072649 Submitted PR [#8](https://github.com/epugh/solr/pull/8) for issues 6 and 7. 6.iii is the only known open issue for now (File copy from local drive to zk fails). Will tackle soon! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] [SOLR-10654] Rename instances of SolrPrometheusExporter to SolrPrometheusFormatter [solr]
dsmiley merged PR #2620: URL: https://github.com/apache/solr/pull/2620 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
dsmiley commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1708537956
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -300,10 +313,11 @@ private VersionedCollectionProps
fetchCollectionProperties(String collection, Wa
throws KeeperException, InterruptedException {
final String znodePath = getCollectionPropsPath(collection);
// lazy init cache cleaner once we know someone is using collection
properties.
-if (collectionPropsCacheCleaner == null) {
- synchronized (zkStateReader.getUpdateLock()) { // Double-checked locking
-if (collectionPropsCacheCleaner == null) {
- collectionPropsCacheCleaner = notifications.submit(new
CacheCleaner());
+if (cacheCleanerThread == null) {
+ synchronized (this) {
+if (cacheCleanerThread == null) {
Review Comment:
Looks good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
dsmiley commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1708536459
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -391,13 +383,7 @@ public void run() {
private class CacheCleaner implements Runnable {
@Override
public void run() {
- while (!Thread.interrupted()) {
-try {
- Thread.sleep(6);
-} catch (InterruptedException e) {
- // Executor shutdown will send us an interrupt
- break;
-}
+ if (!closed) {
Review Comment:
don't even need this condition. The work is not contingent on being closed
or not AFAICT. Even if it did, the pool is going to be closed when it's being
shut down so it's okay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] [SOLR-10654] Rename instances of SolrPrometheusExporter to SolrPrometheusFormatter [solr]
dsmiley commented on PR #2620: URL: https://github.com/apache/solr/pull/2620#issuecomment-2274889982 Thanks for doing this. Indeed; IntelliJ excels at refactorings. I'll merge this into 9.7 tonight. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17391: Cached thread pools actually have more than one thread. [solr]
dsmiley commented on PR #2619: URL: https://github.com/apache/solr/pull/2619#issuecomment-2274878399 Are we good with the CHANGES.txt and other changes? (Pierre or anyone else?) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-10654: Override PrometheusResponseWriter Content Type [solr]
dsmiley merged PR #2616: URL: https://github.com/apache/solr/pull/2616 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1708222059
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/utils/HttpClientUtils.kt:
##
@@ -0,0 +1,43 @@
+/*
+ * 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.solr.composeui.utils
+
+import io.ktor.client.HttpClient
+import io.ktor.client.plugins.contentnegotiation.ContentNegotiation
+import io.ktor.client.plugins.defaultRequest
+import io.ktor.serialization.kotlinx.json.json
+import kotlinx.serialization.json.Json
+
+/**
+ * Function that returns a simple HTTP client that is preconfigured with a base
+ * URL.
+ */
+fun getDefaultClient() = HttpClient {
Review Comment:
> How does authentciation work?
I would probably implement authentication as another UI component that acts
as a decorator for all the underlying components. It would be loaded directly
under the root component in the UI layers (so to speak) and check if there are
credentials (or any other authentication information) present, or if an
authentication is necessary. In case auth data is present, it is used and the
auth component proceeds in the "authenticated route". If there is no data
present, it proceeds with the "unauthenticated route", that shows the user the
login screen.
Additionally, all components with a state have an
[`AppComponentContext`](solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/utils/AppComponentContext.kt)
that can be extended with `authorization` data and user identity. That data
can be used to update the component's state accordingly and hide for example UI
elements if the user is not authorized to use.
So Authentication and Authorization becomes a responsibility of "just
another component" and may be part of the components' context.
The `getDefaultClient()` function is only used for convenience here. The
component responsible for auth would probably provide the logic for creating an
authenticated / authorized client and pass it down to all the other components
that need an `HttpClient`. This way, the component can also react on auth
expiration, token refresh etc, if not handled by the `HttpClient` itself, and
switch the routes on demand.
And last but not least, the Auth component(s) and related store(s) may be
platform-specific or platform-aware, since depending on the platform, different
secure stores may be used for storing credentials / auth data, as well as
different auth mechanisms may be supported depending on the platform the
application is running (like Kerberos auth).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on PR #2605: URL: https://github.com/apache/solr/pull/2605#issuecomment-2274591704 > 2. The biggest downside in the code for me is how boilerplate heavy it is. There are Component interfaces, which have Component implementations, which then reference Models, which come from Stores, which fetch the data via API calls. Those abstractions all make sense individually. But they're the main reason probably that this turned into an 8k line PR! You are right. There are multiple layers that abstract API, logic and UI from each other, resulting in a lot of boilerplate code. The repetitiveness is also a side-effect of applying the same patterns to all componets. Besides the drawback of the quantity in changes, there are two main benefits that may be achieved with that approach: testing components / individual parts becomes much easier, and new developers have an easier start because they can follow the structure and patterns from other components when they are working on a new component. I wanted to write some tests for completion, but I had to postpone it for the time being. >Positing community consensus on taking this forward, I think we'll need to break this into smaller pieces that are less intimidating to folks. I suspect you've already thought a bit about where to draw some of those lines, but I have at least some hunches we could talk over at some point. (Split "Java Properties" and "logging" pages into different PRs, slim the theme down to only support a single mode, commit the theme separately, etc.) You are completely right on that. It is possible, and this was the plan once we work on the Jira part of the migration, to split the components into separate PRs and tickets. This would allow in the long run an easier integration of new components. Contributors will also have to worry only about a single component and not the entire application when working on a ticket, PR, feature or bugfix. The POC includes already two of the components for demonstration, so that it covers almost all parts of the Compose app (including navigation) that are crucial to get started. The only thing I can recall at the moment and that is missing is probably the explicit error handling, but that is something I have already worked on in other projects and shouldn't be a problem integrating. > I've left a few comments inline. Thank you very much for your time reviewing this. As you noticed, this POC still needs some work and cleanup before moving forward. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708121034 ## solr/server/etc/jetty.xml: ## @@ -95,7 +95,7 @@ /solr/* Content-Security-Policy -default-src 'none'; base-uri 'none'; connect-src 'self'; form-action 'self'; font-src 'self'; frame-ancestors 'none'; img-src 'self' data:; media-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self'; worker-src 'self'; +default-src 'none'; base-uri 'none'; connect-src 'self'; form-action 'self'; font-src 'self'; frame-ancestors 'none'; img-src 'self' data:; media-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'wasm-unsafe-eval'; worker-src 'self'; Review Comment: Good thing you pointed this line out. This is one of the security "considerations" that are necessary when introducing a wasm application to our current UI. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1708117136
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/utils/HttpClientUtils.kt:
##
@@ -0,0 +1,43 @@
+/*
+ * 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.solr.composeui.utils
+
+import io.ktor.client.HttpClient
+import io.ktor.client.plugins.contentnegotiation.ContentNegotiation
+import io.ktor.client.plugins.defaultRequest
+import io.ktor.serialization.kotlinx.json.json
+import kotlinx.serialization.json.Json
+
+/**
+ * Function that returns a simple HTTP client that is preconfigured with a base
+ * URL.
+ */
+fun getDefaultClient() = HttpClient {
Review Comment:
Exactly. All classes that need a client define it in the parameters, so that
it can easily be injected wherever necessary. Right now, the method is called
at root level and passed down. The component responsible for authentication
would use the "default client" for the authentication and pass down a differnt
(or the same) client with the necessary headers once authenticated.
It is worth noting that I explicitly avoided to use an injection framework,
because I do not like the "magic" that is happening behind the scenes. It often
caused a lot of confusion, especially to new devs. This approach has also
proven to be easier for testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708109571 ## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/theme/Colors.kt: ## @@ -0,0 +1,766 @@ +/* + * 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.solr.composeui.ui.theme + +import androidx.compose.material3.ColorScheme +import androidx.compose.material3.darkColorScheme +import androidx.compose.material3.lightColorScheme +import androidx.compose.runtime.Composable +import androidx.compose.runtime.Immutable +import androidx.compose.runtime.staticCompositionLocalOf +import androidx.compose.ui.graphics.Color + +/* + * This file holds color values, classes and functions that are used for customizing + * the color scheme of the material theme to match he Solr theme. + * + * Additional colors are also provided as ExtendedColorScheme. This makes it possible + * to use custom color palettes aside the default color scheme that is available. This + * is especially useful when we want to use colors for states of nodes or highlight + * critical errors or warnings. + */ + +val primaryLight = Color(0xFF9C1F00) Review Comment: There are many tools that can generate these values. They are coming from the Material 3 UI components. Material 3 is one of the UI libraries that can be used and is common in Android apps and Google products. Here I used Figma to export the colors / theme I used in the designs, bu that didn't work as expected. You will see some differenes in the colors if you compare it with the Figma designs. So the values would need adjustment to match the designs in Figma. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1708102341
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/main/MainContent.kt:
##
@@ -0,0 +1,78 @@
+/*
+ * 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.solr.composeui.ui.main
+
+import androidx.compose.foundation.layout.Row
+import androidx.compose.foundation.layout.fillMaxHeight
+import androidx.compose.foundation.layout.fillMaxWidth
+import androidx.compose.foundation.layout.padding
+import androidx.compose.foundation.layout.width
+import androidx.compose.foundation.rememberScrollState
+import androidx.compose.foundation.verticalScroll
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.getValue
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.unit.dp
+import com.arkivanov.decompose.extensions.compose.stack.Children
+import com.arkivanov.decompose.extensions.compose.subscribeAsState
+import org.apache.solr.composeui.components.main.MainComponent
+import org.apache.solr.composeui.components.main.integration.asMainMenu
+import org.apache.solr.composeui.ui.environment.EnvironmentContent
+import org.apache.solr.composeui.ui.logging.LoggingContent
+import org.apache.solr.composeui.ui.navigation.NavigationSideBar
+
+/**
+ * The composable used for users that have already authenticated.
+ *
+ * @param component Component that manages the state of the composable.
+ */
+@Composable
+fun MainContent(
+component: MainComponent,
+modifier: Modifier = Modifier,
+) {
+val childStack by component.childStack.subscribeAsState()
+val scrollState = rememberScrollState()
+
+Row(modifier = modifier) {
+NavigationSideBar(
+modifier = Modifier.fillMaxHeight()
+.width(224.dp),
+selectedItem = childStack.active.instance.asMainMenu,
+onNavigate = component::onNavigate,
+)
+
+Children(
+stack = component.childStack,
+modifier = Modifier.weight(1f),
+) {
+when(val child = it.instance) {
+is MainComponent.Child.Environment -> EnvironmentContent(
+component = child.component,
+modifier = Modifier.fillMaxWidth()
+.verticalScroll(scrollState)
+.padding(16.dp),
Review Comment:
The Figma designs I created in advance follow the 8 point grid system
(multiples of 8) that is a common practice for choosing spacings and
dimensions. There are many blog posts that explain various benefits of this
system, [The power of the 8pt grid system in
design](https://medium.com/@mertyagci/the-power-of-the-8pt-grid-system-in-design-1c9dbc683ad8)
and [everything you should know about 8 point grid system in UI
design](https://uxplanet.org/everything-you-should-know-about-8-point-grid-system-in-ux-design-b69cb945b18d)
are just two of them. I mostly follow the guidelines from [Material
3](https://m3.material.io/) too. [Information about
spacing](https://m3.material.io/foundations/layout/understanding-layout/spacing)
and other stuff can be found in the guidelines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1708085072
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/icons/SolrLogo.kt:
##
@@ -0,0 +1,257 @@
+/*
+ * 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.solr.composeui.ui.icons
+
+import androidx.compose.foundation.layout.Box
+import androidx.compose.material.icons.materialPath
+import androidx.compose.material3.Icon
+import androidx.compose.material3.MaterialTheme
+import androidx.compose.runtime.Composable
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.graphics.vector.ImageVector
+import androidx.compose.ui.graphics.vector.rememberVectorPainter
+import androidx.compose.ui.unit.dp
+import org.apache.solr.compose_ui.generated.resources.Res
+import org.apache.solr.compose_ui.generated.resources.cd_solr_logo
+import org.jetbrains.compose.resources.stringResource
+
+/**
+ * Colorized Solr logo.
+ *
+ * @param modifier Modifier that is applied to the wrapper of the logo.
+ */
+@Composable
+fun SolrLogo(
+modifier: Modifier = Modifier,
+) {
+val solrPainter = rememberVectorPainter(
+ImageVector.Builder(
+name = "Logos.SolrLogo",
+defaultWidth = 128.dp,
+defaultHeight = 64.dp,
+viewportWidth = 128f,
+viewportHeight = 64f,
+autoMirror = false,
+).apply {
+materialPath {
+moveTo(25.7013f, 44.178f)
Review Comment:
Since we are working in a "canvas" more or less, drawing with points is
quite common for icons and logos. SVGs have the benefit that they can be
styled, colored and scaled too, which is the reason I chose to use a "native"
drawing approach. I only had to translate the SVG data into Compose, which I
did manually here because of the coloring / theming, but there are also
libraries that can do that for SVG icons.
In UI development, PNGs and JPGs are normally provided in multiple scales to
cover a wide range of screen resolutions to show the image always as sharp as
necessary. SVGs on the other hand can simply be scaled and are therefore
prefered (if available).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1708085072
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/icons/SolrLogo.kt:
##
@@ -0,0 +1,257 @@
+/*
+ * 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.solr.composeui.ui.icons
+
+import androidx.compose.foundation.layout.Box
+import androidx.compose.material.icons.materialPath
+import androidx.compose.material3.Icon
+import androidx.compose.material3.MaterialTheme
+import androidx.compose.runtime.Composable
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.graphics.vector.ImageVector
+import androidx.compose.ui.graphics.vector.rememberVectorPainter
+import androidx.compose.ui.unit.dp
+import org.apache.solr.compose_ui.generated.resources.Res
+import org.apache.solr.compose_ui.generated.resources.cd_solr_logo
+import org.jetbrains.compose.resources.stringResource
+
+/**
+ * Colorized Solr logo.
+ *
+ * @param modifier Modifier that is applied to the wrapper of the logo.
+ */
+@Composable
+fun SolrLogo(
+modifier: Modifier = Modifier,
+) {
+val solrPainter = rememberVectorPainter(
+ImageVector.Builder(
+name = "Logos.SolrLogo",
+defaultWidth = 128.dp,
+defaultHeight = 64.dp,
+viewportWidth = 128f,
+viewportHeight = 64f,
+autoMirror = false,
+).apply {
+materialPath {
+moveTo(25.7013f, 44.178f)
Review Comment:
Since we are working in a "canvas" more or less, drawing with points is
quite common for icons and logos. SVGs have the benefit that they can be
styled, colored and scaled too, which is the reason I chose to use a "native"
drawing approach. I only had to translate the SVG data into Compose, which I
did manually here because of the coloring, but there are also libraries that
can do that for SVG icons.
In UI development, PNGs and JPGs are normally provided in multiple scales to
cover a wide range of screen resolutions to show the image always as sharp as
necessary. SVGs on the other hand can simply be scaled and are therefore
prefered (if available).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708076547 ## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/environment/VersionsCard.kt: ## @@ -0,0 +1,101 @@ +/* + * 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.solr.composeui.ui.environment + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.width +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.ui.Modifier +import androidx.compose.ui.text.style.TextAlign +import androidx.compose.ui.unit.dp +import org.apache.solr.composeui.components.environment.data.JvmData +import org.apache.solr.composeui.components.environment.data.Versions +import org.apache.solr.composeui.ui.components.SolrCard + +/** + * Composable card that displays system values related to versions. + * + * @param versions Solr versions to display. + * @param jvm JVM values to display. + * @param modifier Modifier to apply to the root composable. + */ +@Composable +internal fun VersionsCard( Review Comment: A card is more of a [material card](https://m3.material.io/components/cards/), a styled container in the UI, that "identifies itself" as just another composable. A composable is the equivalent of a UI element / widget you probably have in mind. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708073337 ## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/environment/EnvironmentContent.kt: ## @@ -0,0 +1,123 @@ +/* + * 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.solr.composeui.ui.environment + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column Review Comment: AFAIK they use the same package naming with Jetpack Compose (that is part of the Android ecosystem) to support an easier migration from Jetpack Compose (e.g. android app) to a multiplatform project (Compose MP). If I am not mistaken, they also map many of the classes for the android target and use the Jetpack Compose implementation if there is no multiplatform one. That way, the Jetpack Compose documentation, examples, tutorials and so on can be applied to Compose MP too. But I am not 100% sure, so don't take my word 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. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1708066620
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/components/main/MainComponent.kt:
##
@@ -0,0 +1,91 @@
+/*
+ * 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.solr.composeui.components.main
+
+import com.arkivanov.decompose.router.stack.ChildStack
+import com.arkivanov.decompose.value.Value
+import kotlinx.coroutines.flow.StateFlow
+import org.apache.solr.composeui.components.environment.EnvironmentComponent
+import org.apache.solr.composeui.components.logging.LoggingComponent
+import org.apache.solr.composeui.components.navigation.NavigationComponent
+import org.apache.solr.composeui.ui.navigation.MainMenu
+
+/**
+ * Main component of the application that is used as base for users with
access.
+ *
+ * Note that this component can be accessed if the user is either
authenticated or if the Solr
+ * instance accessed does not have any authentication enabled.
+ */
+interface MainComponent : NavigationComponent {
+
+/**
+ * Child stack that holds the navigation state.
+ */
+val childStack: Value>
+
+/**
+ * Handles navigation requests from a navigation menu.
+ *
+ * @param menuItem The destination to navigate to.
+ */
+fun onNavigate(menuItem: MainMenu)
+
+/**
+ * Child interface that defines all available children of the
[MainComponent].
+ */
+sealed interface Child {
+
+// TODO Uncomment once DashboardComponent available
Review Comment:
Placeholders for the planned UI screens / components, yes.
Since this is just a POC, I added a few commented out lines to show how it
would look if the other screens would be added, so that it makes more sense
when looking at the child interface. The navigation, which the Child interface
is part of, is one of the more complicated parts in the UI, since it has to
handle back and forth navigation and update the components' states accordingly
(like pause running background tasks, stop animations and so on).
The libraries' documentation provide a more detailed overview and
explaination 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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708065517 ## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/components/environment/integration/Mappers.kt: ## Review Comment: I am not sure if your question is about their usage, but the mapper functions are used by the components to map the store's state to a component model / component state. They are used in the components' implementation and could theoretically be inside the component's implementation. However, they are placed in a shared place for each component, so that multiple implementations of the same component interface could make use of them if necessary. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708063991 ## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/components/environment/data/JavaPropertiesResponse.kt: ## @@ -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.solr.composeui.components.environment.data + +import kotlinx.serialization.SerialName +import kotlinx.serialization.Serializable + +/** + * Response class of the `java-properties` API endpoint. Review Comment: 1. Exactly. 2. If we would have a Solr client or API that would provide these data classes, we could remove any classes related to the API. These API data classes are also mapped to avoid any direct dependencies that would break multiple places if the API change in the future. This way we have a clean separation of concern and we could reduce further the visibilities of the classes (e.g. only `HttpEnvironmentStoreClient` would know the API classes) UI projects usually have two modules, one for the UI and one for the logic. Some people may consider spliting it further based on the logical grouping of components, like creating a separate module for only `auth` related components. Since I wanted to keep everything in a single module, which should be discussed, I made the separation of UI and non-ui components with packages (`org.apache.solr.composeui.components` for logic and `org.apache.solr.composeui.ui` for UI). This is also the reason why there are so many "insignificant" interfaces. If we would have two modules, the abstractions would be necessary and the UI would know / see the interfaces, but not the implementations. Another benefit of the interfaces is for providing test or preview implementations for UI testing and previews that can be rendered in the IDE without launching the app. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
epugh commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708064021 ## solr/compose-ui/src/commonMain/composeResources/font/firacode-variable.ttf: ## Review Comment: also, I would't go through heroics to match the existing UI styles... It's not like we had "The worlds Greatest UX and UI people come up wiht a special language of UI and UX just ofr Solr"...Anything jthat looks nice is fine by me. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
epugh commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1708062769
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/utils/HttpClientUtils.kt:
##
@@ -0,0 +1,43 @@
+/*
+ * 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.solr.composeui.utils
+
+import io.ktor.client.HttpClient
+import io.ktor.client.plugins.contentnegotiation.ContentNegotiation
+import io.ktor.client.plugins.defaultRequest
+import io.ktor.serialization.kotlinx.json.json
+import kotlinx.serialization.json.Json
+
+/**
+ * Function that returns a simple HTTP client that is preconfigured with a base
+ * URL.
+ */
+fun getDefaultClient() = HttpClient {
Review Comment:
this is something that I personally is really important to see... How does
authentciation 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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708060169 ## solr/compose-ui/src/commonMain/composeResources/font/firacode-variable.ttf: ## Review Comment: It is possible to use a dependency and load the fonts "dynamically" if there is one for Compose MP (I think I saw something). I am not a fan to version fonts either, I just did it here (I have no reason why, I just tried to mitigate the look and feel from the new designs). And using custom fonts in web has always been a "complicated" topic (and already caused some issues), so avoiding it completely may also prove to be a better option. So this is definitely something that needs further investigation and should be addressed. Good thing you pointed that out. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708057897 ## solr/compose-ui/build.gradle.kts: ## @@ -0,0 +1,134 @@ +/* Review Comment: I was considering migrating to Kotlin DSL if we start using Kotlin in general. But if there are still limitations in IDE support, or like I was told some "relationships" to the Lucene project prefer Groovy DSL, then these files should be changed to Groovy DSL. Either way, it should probably be discussed and unified before or short after the resolution of the POC, depending on the outcome. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1708055052
##
solr/api/build.gradle:
##
@@ -16,8 +18,8 @@
*/
plugins {
- id 'io.swagger.core.v3.swagger-gradle-plugin' version '2.2.2'
- id "org.openapi.generator" version "6.0.1"
+ alias(libs.plugins.swagger3Core)
Review Comment:
As mentioned above, the changes came from a previous attempt to create a
Solr client for auto-generating classes from the OAS. But I dropped it since it
was out of scope. So these changes could be reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708054252 ## gradle/libs.versions.toml: ## @@ -0,0 +1,65 @@ +[versions] Review Comment: I am currently working on a migration to use version catalogs in the entire project (outside this POC). I will probably discuss version catalogs soon in the mailing list. I saw it in the Lucene project and I am using version catalogs on personal projects too, so it was the easiest and quickest way to get started. But it should definitely be either-or, not both present like in this POC. See https://github.com/apache/lucene/pull/13484. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708053334 ## dev-docs/ui/technology-overview.adoc: ## @@ -0,0 +1,83 @@ += Technology Overview Review Comment: I listed all libraries here in detail, even though some of them are common libraries in Kotlin / Compose projects. I did this only because most of the libraries are for non-kotlin folks new, so having some reference should be helpful for getting started. Maybe in the long run it doesn't have to be that detailed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1708050511
##
build.gradle:
##
@@ -20,6 +20,10 @@ import java.time.format.DateTimeFormatter
plugins {
id 'base'
+ alias(libs.plugins.kotlinMultiplatform) apply false
Review Comment:
I added the plugins in the project gradle so that they are loaded once
(currently used by two modules with different versions).
These changes are not necessary as such and came from a draft I made before
but forgot to clean up. The draft was implementing a Solr client for Kotlin for
generating classes from OAS, and I had to sync the OpenAPI plugin versions.
If I remember correct, the plugin for Kotlin multiplatform and Compose
should be loaded at project level (haven't tried it at module level) and only
applied to modules were necessary.
The multiplatform plugin is used for supporting individual targets in the
gradle module level (hence the `commonMain`, `jvmMain`, `wasmJsMain`
sub-modules / directories in the new ui module). This way, we can provide
platform specific code if necessary (see [expect-actual
declarations](https://kotlinlang.org/docs/multiplatform-expect-actual.html)).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
aparnasuresh85 commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1708045290
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -300,10 +313,11 @@ private VersionedCollectionProps
fetchCollectionProperties(String collection, Wa
throws KeeperException, InterruptedException {
final String znodePath = getCollectionPropsPath(collection);
// lazy init cache cleaner once we know someone is using collection
properties.
-if (collectionPropsCacheCleaner == null) {
- synchronized (zkStateReader.getUpdateLock()) { // Double-checked locking
-if (collectionPropsCacheCleaner == null) {
- collectionPropsCacheCleaner = notifications.submit(new
CacheCleaner());
+if (cacheCleanerThread == null) {
+ synchronized (this) {
+if (cacheCleanerThread == null) {
Review Comment:
Ok using ScheduledThreadPoolExecutor works. I updated the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
epugh commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708030153 ## solr/server/etc/jetty.xml: ## @@ -95,7 +95,7 @@ /solr/* Content-Security-Policy -default-src 'none'; base-uri 'none'; connect-src 'self'; form-action 'self'; font-src 'self'; frame-ancestors 'none'; img-src 'self' data:; media-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self'; worker-src 'self'; +default-src 'none'; base-uri 'none'; connect-src 'self'; form-action 'self'; font-src 'self'; frame-ancestors 'none'; img-src 'self' data:; media-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'wasm-unsafe-eval'; worker-src 'self'; Review Comment: `wasm`! Fun! I heart that tech. ;-) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on PR #2599: URL: https://github.com/apache/solr/pull/2599#issuecomment-2274460985 > Nice! Nearly ready to commit IMO; needs a CHANGES.txt (which I could add if needed). 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. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
epugh commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1708029278 ## dev-docs/ui/technology-overview.adoc: ## @@ -0,0 +1,83 @@ += Technology Overview Review Comment: I think "sheer number" is okay, have you ever looked at a React app? "Now downloading library 234 of 789" -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Merge to production [solr-site]
janhoy merged PR #120: URL: https://github.com/apache/solr-site/pull/120 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17295 Add download link for OpenAPI [solr-site]
janhoy merged PR #119: URL: https://github.com/apache/solr-site/pull/119 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
dsmiley commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1707774326
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -300,10 +313,11 @@ private VersionedCollectionProps
fetchCollectionProperties(String collection, Wa
throws KeeperException, InterruptedException {
final String znodePath = getCollectionPropsPath(collection);
// lazy init cache cleaner once we know someone is using collection
properties.
-if (collectionPropsCacheCleaner == null) {
- synchronized (zkStateReader.getUpdateLock()) { // Double-checked locking
-if (collectionPropsCacheCleaner == null) {
- collectionPropsCacheCleaner = notifications.submit(new
CacheCleaner());
+if (cacheCleanerThread == null) {
+ synchronized (this) {
+if (cacheCleanerThread == null) {
Review Comment:
fetchCollectionProperties: need to know if there is a task in the executor
or not. If we use the ScheduledThreadPoolExecutor type, then we have methods
like `getTaskCount` to know if something is scheduled.
An alternative (maybe simpler?) is to use Timer.
Wether the ScheduledExecutor or Timer, could also use a null instance
initially (not final, of course) to differentiate not-started from started
which is totally fine too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
aparnasuresh85 commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1707659461
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -300,10 +313,11 @@ private VersionedCollectionProps
fetchCollectionProperties(String collection, Wa
throws KeeperException, InterruptedException {
final String znodePath = getCollectionPropsPath(collection);
// lazy init cache cleaner once we know someone is using collection
properties.
-if (collectionPropsCacheCleaner == null) {
- synchronized (zkStateReader.getUpdateLock()) { // Double-checked locking
-if (collectionPropsCacheCleaner == null) {
- collectionPropsCacheCleaner = notifications.submit(new
CacheCleaner());
+if (cacheCleanerThread == null) {
+ synchronized (this) {
+if (cacheCleanerThread == null) {
Review Comment:
I tried this. Using the following code, where cacheCleanerExecutor is
defined as
```
private final ScheduledExecutorService cacheCleanerExecutor =
Executors.newSingleThreadScheduledExecutor(new
SolrNamedThreadFactory("cacheCleaner"));
```
and then used in `fetchCollectionProperties()` as follows:
```
if (cacheCleanerExecutor.isShutdown()) {
synchronized (this) {
if (cacheCleanerExecutor.isShutdown()) {
cacheCleanerExecutor.scheduleAtFixedRate(new CacheCleaner(), 0, 1,
TimeUnit.MINUTES);
}
}
}
```
I ran a Junit test and set a breakpoint at
`cacheCleanerExecutor.scheduleAtFixedRate`, it is never hit.
But when I changed to using a state variable like the following, where
`collectionPropsCacheCleanerInitialized` is an `AtomicBoolean`, I see the
breakpoint being hit to schedule the cacheCleaner thread
```
if (collectionPropsCacheCleanerInitialized.compareAndSet(false, true)) {
synchronized (this) {
cacheCleanerExecutor.scheduleAtFixedRate(new CacheCleaner(), 0, 1,
TimeUnit.MINUTES);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
aparnasuresh85 commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1707659461
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -300,10 +313,11 @@ private VersionedCollectionProps
fetchCollectionProperties(String collection, Wa
throws KeeperException, InterruptedException {
final String znodePath = getCollectionPropsPath(collection);
// lazy init cache cleaner once we know someone is using collection
properties.
-if (collectionPropsCacheCleaner == null) {
- synchronized (zkStateReader.getUpdateLock()) { // Double-checked locking
-if (collectionPropsCacheCleaner == null) {
- collectionPropsCacheCleaner = notifications.submit(new
CacheCleaner());
+if (cacheCleanerThread == null) {
+ synchronized (this) {
+if (cacheCleanerThread == null) {
Review Comment:
I tried this. Using the following code, where cacheCleanerExecutor is
defined as
```
private final ScheduledExecutorService cacheCleanerExecutor =
Executors.newSingleThreadScheduledExecutor(new
SolrNamedThreadFactory("cacheCleaner"));
```:
and then used in `fetchCollectionProperties()` as follows:
```
if (cacheCleanerExecutor.isShutdown()) {
synchronized (this) {
if (cacheCleanerExecutor.isShutdown()) {
cacheCleanerExecutor.scheduleAtFixedRate(new CacheCleaner(), 0, 1,
TimeUnit.MINUTES);
}
}
}
```
I ran a Junit test and set a breakpoint at
`cacheCleanerExecutor.scheduleAtFixedRate`, it is never hit.
But when I changed to using a state variable like the following, I see the
breakpoint being hit to schedule the cacheCleaner thread
```
if (collectionPropsCacheCleanerInitialized.compareAndSet(false, true)) {
synchronized (this) {
cacheCleanerExecutor.scheduleAtFixedRate(new CacheCleaner(), 0, 1,
TimeUnit.MINUTES);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
aparnasuresh85 commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1707659461
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -300,10 +313,11 @@ private VersionedCollectionProps
fetchCollectionProperties(String collection, Wa
throws KeeperException, InterruptedException {
final String znodePath = getCollectionPropsPath(collection);
// lazy init cache cleaner once we know someone is using collection
properties.
-if (collectionPropsCacheCleaner == null) {
- synchronized (zkStateReader.getUpdateLock()) { // Double-checked locking
-if (collectionPropsCacheCleaner == null) {
- collectionPropsCacheCleaner = notifications.submit(new
CacheCleaner());
+if (cacheCleanerThread == null) {
+ synchronized (this) {
+if (cacheCleanerThread == null) {
Review Comment:
I tried this. Using the following code, where cacheCleanerExecutor is
defined as
```
private final ScheduledExecutorService cacheCleanerExecutor =
Executors.newSingleThreadScheduledExecutor(new
SolrNamedThreadFactory("cacheCleaner"));
```
and then used in `fetchCollectionProperties()` as follows:
```
if (cacheCleanerExecutor.isShutdown()) {
synchronized (this) {
if (cacheCleanerExecutor.isShutdown()) {
cacheCleanerExecutor.scheduleAtFixedRate(new CacheCleaner(), 0, 1,
TimeUnit.MINUTES);
}
}
}
```
I ran a Junit test and set a breakpoint at
`cacheCleanerExecutor.scheduleAtFixedRate`, it is never hit.
But when I changed to using a state variable like the following, I see the
breakpoint being hit to schedule the cacheCleaner thread
```
if (collectionPropsCacheCleanerInitialized.compareAndSet(false, true)) {
synchronized (this) {
cacheCleanerExecutor.scheduleAtFixedRate(new CacheCleaner(), 0, 1,
TimeUnit.MINUTES);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
aparnasuresh85 commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1707659461
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -300,10 +313,11 @@ private VersionedCollectionProps
fetchCollectionProperties(String collection, Wa
throws KeeperException, InterruptedException {
final String znodePath = getCollectionPropsPath(collection);
// lazy init cache cleaner once we know someone is using collection
properties.
-if (collectionPropsCacheCleaner == null) {
- synchronized (zkStateReader.getUpdateLock()) { // Double-checked locking
-if (collectionPropsCacheCleaner == null) {
- collectionPropsCacheCleaner = notifications.submit(new
CacheCleaner());
+if (cacheCleanerThread == null) {
+ synchronized (this) {
+if (cacheCleanerThread == null) {
Review Comment:
I tried this. Using the following code, where cacheCleanerExecutor is
defined as
```
private final ScheduledExecutorService cacheCleanerExecutor =
Executors.newSingleThreadScheduledExecutor(new
SolrNamedThreadFactory("cacheCleaner"));
```:
and then used in `fetchCollectionProperties()` as follows:
```
if (cacheCleanerExecutor.isShutdown()) {
synchronized (this) {
if (cacheCleanerExecutor.isShutdown()) {
cacheCleanerExecutor.scheduleAtFixedRate(new CacheCleaner(), 0, 1,
TimeUnit.MINUTES);
}
}
}
```
I ran a Junit test and set a breakpoint at
`cacheCleanerExecutor.scheduleAtFixedRate`, it is never hit.
But when I changed to using a state variable like the following, I see the
breakpoint being hit to schedule the cacheCleaner thread
```
if (collectionPropsCacheCleanerInitialized.compareAndSet(false, true)) {
synchronized (this) {
cacheCleanerExecutor.scheduleAtFixedRate(new CacheCleaner(), 0, 1,
TimeUnit.MINUTES);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
aparnasuresh85 commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1707659461
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -300,10 +313,11 @@ private VersionedCollectionProps
fetchCollectionProperties(String collection, Wa
throws KeeperException, InterruptedException {
final String znodePath = getCollectionPropsPath(collection);
// lazy init cache cleaner once we know someone is using collection
properties.
-if (collectionPropsCacheCleaner == null) {
- synchronized (zkStateReader.getUpdateLock()) { // Double-checked locking
-if (collectionPropsCacheCleaner == null) {
- collectionPropsCacheCleaner = notifications.submit(new
CacheCleaner());
+if (cacheCleanerThread == null) {
+ synchronized (this) {
+if (cacheCleanerThread == null) {
Review Comment:
I tried this. Using the following code, where cacheCleanerExecutor is
defined as
```
private final ScheduledExecutorService cacheCleanerExecutor =
Executors.newSingleThreadScheduledExecutor(new
SolrNamedThreadFactory("cacheCleaner"));
```:
and then used in `fetchCollectionProperties()` as follows:
```
if (cacheCleanerExecutor.isShutdown()) {
synchronized (this) {
if (cacheCleanerExecutor.isShutdown()) {
cacheCleanerExecutor.scheduleAtFixedRate(new CacheCleaner(), 0, 1,
TimeUnit.MINUTES);
}
}
}
```
I ran a Junit test and set a breakpoint at
`cacheCleanerExecutor.scheduleAtFixedRate`, it is never hit.
But when I changed to using a state variable like the following, I see the
breakpoint being hit to schedule the cacheCleaner thread
```
if (collectionPropsCacheCleanerInitialized.compareAndSet(false, true)) {
synchronized (this) {
cacheCleanerExecutor.scheduleAtFixedRate(new CacheCleaner(), 0, 1,
TimeUnit.MINUTES);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
dsmiley commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1707539294
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -116,9 +118,19 @@ public void getClusterStatus(NamedList results)
// add live_nodes
if (withLiveNodes) clusterStatus.add("live_nodes", liveNodes);
}
+
+Aliases aliases = null;
+if (withCollection || withAliases) {
Review Comment:
Since they occupy the same namespace, I could see returning aliases if you
want collections.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17391: Cached thread pools actually have more than one thread. [solr]
dsmiley commented on PR #2619: URL: https://github.com/apache/solr/pull/2619#issuecomment-2273984102 After reading ThreadPoolExecutor's docs a bit more, I was surprised to see that a new thread will be created for a new task when the thread count is less than the core size _even if there are threads idle_. I checked out this PR and did a trivial experiment where I executed two tasks in series (one completing before the other), even with a sleep in-between, and both reported different threads. That's sad and makes no sense to me why it would be designed that way (or maybe it's more like a documented deficiency?). Any way, It's not that important; not too many idle threads. I suppose if the idle time were really short (a second?) then there would be no wasted idle threads as they would barely exist. But it would mean more thread creation over time and I'm not sure what to think of that. Any way, I think this is a nice change. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
gerlowskija commented on PR #2605: URL: https://github.com/apache/solr/pull/2605#issuecomment-2273958773 Also - when I check out this code locally it compiles fine in my IDE, but I'm having trouble running the packaging tasks (e.g. `./gradlew assemble`) in my terminal. The gradle build hangs on several of the compose-ui tasks. In the example below I let it run > 50m before killing the build: ``` > Task :solr:compose-ui:compileKotlinWasmJs Options for KOTLIN DAEMON: IncrementalCompilationOptions(super=CompilationOptions(compilerMode=INCREMENTAL_COMPILER, targetPlatform=JS, reportCategories=[0, 3], reportSeverity=2, requestedCompilationResults=[0], kotlinScriptExtensions=[]), areFileChangesKnown=false, modifiedFiles=null, deletedFiles=null, classpathChanges=NotAvailableForJSCompiler, workingDir=/Users/gerlowskija/checkouts/solr/solr/compose-ui/build/kotlin/compileKotlinWasmJs/cacheable, multiModuleICSettings=MultiModuleICSettings(buildHistoryFile=/Users/gerlowskija/checkouts/solr/solr/compose-ui/build/kotlin/compileKotlinWasmJs/local-state/build-history.bin, useModuleDetection=false), usePreciseJavaTracking=false, icFeatures=IncrementalCompilationFeatures(withAbiSnapshot=false, preciseCompilationResultsBackup=true, keepIncrementalCompilationCachesInMemory=true, enableUnsafeIncrementalCompilationForMultiplatform=false), outputFiles=[/Users/gerlowskija/checkouts/solr/solr/compose-ui/build/classes/kotlin/wasmJs/main, /U sers/gerlowskija/checkouts/solr/solr/compose-ui/build/kotlin/compileKotlinWasmJs/cacheable, /Users/gerlowskija/checkouts/solr/solr/compose-ui/build/kotlin/compileKotlinWasmJs/local-state]) <===--> 90% EXECUTING [51m 4s] > IDLE > IDLE > :solr:compose-ui:compileCommonMainKotlinMetadata > :solr:compose-ui:compileKotlinDesktop > :solr:compose-ui:compileKotlinWasmJs > IDLE ``` Going to try this on different machines and hopefully work around it. But if anyone has any insights (or even specific commands that worked for them), I'd love to get this working locally to kick the tires of the new UI. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14414: Introduce new UI (SIP-7) [solr]
gerlowskija commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1707007092 ## dev-docs/ui/testing-and-deployment.adoc: ## @@ -0,0 +1,19 @@ += Testing and Deployment + +== Testing + Review Comment: [0] Not important, just leaving this comment as a reminder that these docs should be populated at some point if this moves beyond POC. ## dev-docs/ui/component-development.adoc: ## @@ -0,0 +1,95 @@ += Component Development +:toc: left + +== Overview + +The following list contains a possible approach for implementing a new UI component: + +1. Create new design or start with an existing design, see for example Figma +2. Validate the use case and analyze the components that may be used for the implementation +3. Create Composables that represent the UI component(s) and use placeholders for data population +4. Create a component interface and implementation with the UI state and UI component interactions +5. Create previews with preview component implementations to check the UI implementation +6. Create a store and store provider for fetching resources and interacting with Solr backend +7. Implement the client used by the store provider +8. Write test and test the new component +9. If not already done, integrate component in the existing application +10. If not already done, extract resources like texts to allow internationalization and localization + +It is recommended to take a look at existing components, so that you get a better understanding +on how things are implemented, what they have in common, and how each technology is utilized. + +== Component's Logic + +=== Components (Decompose) + +The component integration interacts with the UI composables and the state store. + +The implementation of the component interface "catches" user inputs like clicks and passes them +to the store as `Intent`s. The intents are then handled by the store implementation and +may send a request to the backend and / or update the store state. The component is consuming +and mapping the store state to the UI state. So once the store state is updated, it will +reflect the changes in the UI. + +=== State Stores and Store Providers + +The state stores manage the state of the application, but independent of the state that is +represented in the UI. Instances are created by store providers that hold the logic of the +store. + +Store providers consist of an executor implementation that consumes actions and intents and +creates messages and labels, a reducer that updates the store state with the messages produced +by the executor, and a function for retrieving an instance of the store. + +The store provider does also define the interface for the client that has to be provided in +order for the executor to make API calls and interact with the Solr backend. + +== Component's Visuals + +=== Composables + +Composables are the UI elements that are defined and styled. They can be seen as boxes, rows and +columns that are nested and change their style and structure based on conditions, state and input. + +There are many ways to get started, but the easiest way probably is to get familiar with the basics +and try things out. The Figma designs make use of almost the same elements for designing, +so the structure and configurations there may be mapped almost one-by-one in Compose code. + +=== Styling + +The styling in Compose is done via `Modifier`s. Each composable should normally accept a modifier +as parameter, so that the user can customize specific visual parameters of the composable like +width, height and alignment in the parent's composable. + +Since we are using Material 3, you do not have to care much about colors, typography and shapes. +These are configured for the entire app, and you only have to make use of the right properties +that are provided by the theme. + +=== Accessibility + +Compose comes with many accessibility features that can be used to improve the user experience. + +The simplest form of accessibility in UI is probably the responsiveness of the UI. this is +realized with `WindowSizeClass`. Some compsables may use a wrapper (usually suffixed with `Content`) Review Comment: [0] wanted to flag the typo in "compsables" in case this PR comes out of draft mode at some point. ## solr/compose-ui/README.md: ## @@ -0,0 +1,32 @@ +# Compose Admin UI Review Comment: [+1] Great docs, thank you! ## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/components/environment/data/JavaPropertiesResponse.kt: ## @@ -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
[PR] chore(deps): update dependency org.apache.commons:commons-lang3 to v3.16.0 [solr]
solrbot opened a new pull request, #2628: URL: https://github.com/apache/solr/pull/2628 This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [org.apache.commons:commons-lang3](https://commons.apache.org/proper/commons-lang/) ([source](https://gitbox.apache.org/repos/asf?p=commons-lang.git)) | dependencies | minor | `3.15.0` -> `3.16.0` | --- ### Configuration **Schedule**: Branch creation - "* * * * *" (UTC), Automerge - At any time (no schedule defined). **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/solrbot/renovate-github-action) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17253 Release wizard suggests wrong minor version for jira [solr]
janhoy merged PR #2622: URL: https://github.com/apache/solr/pull/2622 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17245 Download KEYS from downloads server [solr]
janhoy merged PR #2623: URL: https://github.com/apache/solr/pull/2623 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-16996: remove no-longer-applicable solr/licenses/argparse4j-* files [solr]
epugh commented on PR #2627: URL: https://github.com/apache/solr/pull/2627#issuecomment-2273769080 I wonder if this is important enough to bring to branch_9_7? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-16996: remove no-longer-applicable solr/licenses/argparse4j-* files [solr]
epugh merged PR #2627: URL: https://github.com/apache/solr/pull/2627 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-16996: remove no-longer-applicable solr/licenses/argparse4j-* files [solr]
epugh commented on PR #2627: URL: https://github.com/apache/solr/pull/2627#issuecomment-2273767606 Like wise! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1707163750
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -116,9 +118,19 @@ public void getClusterStatus(NamedList results)
// add live_nodes
if (withLiveNodes) clusterStatus.add("live_nodes", liveNodes);
}
+
+Aliases aliases = null;
+if (withCollection || withAliases) {
Review Comment:
Whether a `collection` parameter is passed (where the input could be either
a collection or an alias) or if `includeAll` is set to `true`, aliases are
retrieved from `ZkStateReader` to create a mapping of collections to the list
of aliases associated with them and returned as part of the response. In this
case and/or if `withAliases=true`, the set of aliases is read once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1707163750
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -116,9 +118,19 @@ public void getClusterStatus(NamedList results)
// add live_nodes
if (withLiveNodes) clusterStatus.add("live_nodes", liveNodes);
}
+
+Aliases aliases = null;
+if (withCollection || withAliases) {
Review Comment:
Whether a collection parameter is passed (where the input could be either a
collection or an alias) or if includeAll is set to true, aliases are retrieved
from ZkStateReader to create a mapping of collections to the list of aliases
associated with them and returned as part of the response. In this case and/or
if `withAliases=true`, the set of aliases is read once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1707163750
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -116,9 +118,19 @@ public void getClusterStatus(NamedList results)
// add live_nodes
if (withLiveNodes) clusterStatus.add("live_nodes", liveNodes);
}
+
+Aliases aliases = null;
+if (withCollection || withAliases) {
Review Comment:
Whether a collection parameter is passed (where the input could be either a
collection or an alias) or if includeAll is set to true, aliases are retrieved
from ZkStateReader to create a mapping of collections to the list of aliases
associated with them and returned as part of the response.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
dsmiley commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1707112138
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -116,9 +118,19 @@ public void getClusterStatus(NamedList results)
// add live_nodes
if (withLiveNodes) clusterStatus.add("live_nodes", liveNodes);
}
+
+Aliases aliases = null;
+if (withCollection || withAliases) {
Review Comment:
why withCollection?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1707086097
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -191,43 +234,23 @@ public void getClusterStatus(NamedList results)
}
String configName = clusterStateCollection.getConfigName();
collectionStatus.put("configName", configName);
- if (message.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
+ if (solrParams.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
PerReplicaStates prs = clusterStateCollection.getPerReplicaStates();
collectionStatus.put("PRS", prs);
}
collectionProps.add(name, collectionStatus);
}
-List liveNodes =
-
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
-
-// now we need to walk the collectionProps tree to cross-check replica
state with live nodes
-crossCheckReplicaStateWithLiveNodes(liveNodes, collectionProps);
-
-NamedList clusterStatus = new SimpleOrderedMap<>();
-clusterStatus.add("collections", collectionProps);
-
-// read cluster properties
-Map clusterProps = zkStateReader.getClusterProperties();
-if (clusterProps != null && !clusterProps.isEmpty()) {
- clusterStatus.add("properties", clusterProps);
-}
-
// add the alias map too
Map collectionAliasMap = aliases.getCollectionAliasMap();
// comma delim
if (!collectionAliasMap.isEmpty()) {
clusterStatus.add("aliases", collectionAliasMap);
Review Comment:
added another parameter to fetch all aliases, updated the adoc for
CLUSTERSTATUS
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1707084935
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##
@@ -100,7 +100,13 @@ public class CloudHttp2SolrClientTest extends
SolrCloudTestCase {
private static final Pattern PATTERN_WITHOUT_COLLECTION =
Pattern.compile(
"path=/admin/collections.*params=\\{[^}]*action=CLUSTERSTATUS"
- + "(?![^}]*collection=)[^}]*\\}");
+ + "(?=[^}]*includeAll=true)"
Review Comment:
Updated test doesnt include regexes anymore.
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##
@@ -84,6 +84,11 @@ public void init(List solrUrls) throws Exception {
/** Create a SolrClient implementation that uses the specified Solr node URL
*/
protected abstract SolrClient getSolrClient(String baseUrl);
+ @Override
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1707084246
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##
@@ -291,7 +297,7 @@ public void testHttpCspPerf() throws Exception {
// lookup)
assertLogCount(collectionClusterStateLogs, 1);
// 1 call to fetch entire cluster state from HttpCSP.fetchLiveNodes()
- assertLogCount(entireClusterStateLogs, 1);
+ assertLogCount(entireClusterStateLogs, 0);
Review Comment:
I agree. Improved the 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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-16996: Update Solr Exporter for Prometheus cli to use commons-cli [solr]
cpoerschke commented on code in PR #2602: URL: https://github.com/apache/solr/pull/2602#discussion_r1707047176 ## versions.props: ## @@ -29,7 +29,6 @@ io.prometheus:*=0.16.0 io.swagger.core.v3:*=2.2.22 jakarta.ws.rs:jakarta.ws.rs-api=3.1.0 junit:junit=4.13.2 -net.sourceforge.argparse4j:argparse4j=0.9.0 Review Comment: The corresponding `licenses` files can be removed now then too I think, opened #2627 for 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. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] SOLR-16996: remove no-longer-applicable solr/licenses/argparse4j-* files [solr]
cpoerschke opened a new pull request, #2627: URL: https://github.com/apache/solr/pull/2627 #2602 follow-up https://issues.apache.org/jira/browse/SOLR-16996 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-10654: Override PrometheusResponseWriter Content Type [solr]
dsmiley commented on code in PR #2616:
URL: https://github.com/apache/solr/pull/2616#discussion_r1707038840
##
solr/core/src/java/org/apache/solr/response/PrometheusResponseWriter.java:
##
@@ -49,7 +49,7 @@
*/
@SuppressWarnings(value = "unchecked")
public class PrometheusResponseWriter extends RawResponseWriter {
- static String CONTENT_TYPE_PROMETHEUS = "text/plain; version=0.0.4";
+ private final String CONTENT_TYPE_PROMETHEUS = "text/plain; version=0.0.4";
Review Comment:
```suggestion
private static final String CONTENT_TYPE_PROMETHEUS = "text/plain;
version=0.0.4";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[PR] PeerSync: INFO-log DBQs with NOW [solr]
cpoerschke opened a new pull request, #2626: URL: https://github.com/apache/solr/pull/2626 (no comment) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] SOLR-17298 Add tests to verify that graph, join and rerank queries are unaffected by the use of multithreaded search and re-enable them. [solr]
gus-asf opened a new pull request, #2625: URL: https://github.com/apache/solr/pull/2625 https://issues.apache.org/jira/browse/SOLR-17298 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17249 Edit .asf.yaml on main branch [solr]
janhoy commented on PR #2624: URL: https://github.com/apache/solr/pull/2624#issuecomment-2273467112 Tested locally, both for a major release and a minor release. Output for a major release: ``` Edit `.asf.yaml` to protect new git branch(es) branch_10_0 and branch_10x. Run these commands to edit `.asf.yaml` and protect the new branch(es) cd ~/.solr-releases/10.0.0/solr git checkout main git pull --ff-only # Add the newly created branch(es) branch_10_0 and branch_10x under `protected_branches` in `.asf.yaml`. An editor will open. subl .asf.yaml git add .asf.yaml && git commit -m "Add branch protection for branch_10_0 and branch_10x" && git push You will get prompted before running each individual command. Q: Do you want me to run these commands now? (y/n): ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Update io.netty:* to v4.1.112.Final [solr]
janhoy commented on PR #2592: URL: https://github.com/apache/solr/pull/2592#issuecomment-2273330359 Merging this to `branch_9x` (for 9.8), not backporting to new `branch_9_7` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Update io.netty:* to v4.1.112.Final [solr]
janhoy merged PR #2592: URL: https://github.com/apache/solr/pull/2592 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Update gradle/wrapper-validation-action action to v3 [solr]
janhoy merged PR #2578: URL: https://github.com/apache/solr/pull/2578 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-10654: Override PrometheusResponseWriter Content Type [solr]
dsmiley commented on code in PR #2616:
URL: https://github.com/apache/solr/pull/2616#discussion_r1706394845
##
solr/core/src/java/org/apache/solr/response/PrometheusResponseWriter.java:
##
@@ -49,6 +49,7 @@
*/
@SuppressWarnings(value = "unchecked")
public class PrometheusResponseWriter extends RawResponseWriter {
+ static String CONTENT_TYPE_PROMETHEUS = "text/plain; version=0.0.4";
Review Comment:
obviously should be private and final.
IMO is also a pointless constant but we have different preferences on this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
dsmiley commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1706392465
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -300,10 +313,11 @@ private VersionedCollectionProps
fetchCollectionProperties(String collection, Wa
throws KeeperException, InterruptedException {
final String znodePath = getCollectionPropsPath(collection);
// lazy init cache cleaner once we know someone is using collection
properties.
-if (collectionPropsCacheCleaner == null) {
- synchronized (zkStateReader.getUpdateLock()) { // Double-checked locking
-if (collectionPropsCacheCleaner == null) {
- collectionPropsCacheCleaner = notifications.submit(new
CacheCleaner());
+if (cacheCleanerThread == null) {
+ synchronized (this) {
+if (cacheCleanerThread == null) {
Review Comment:
If you want.
But I think the best solution reflecting the pattern/need for this cache
cleaner is a ScheduledExecutorService and no extra state var. all
ExecutorService's *have* state or could use null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
dsmiley commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1706383802
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##
@@ -291,7 +297,7 @@ public void testHttpCspPerf() throws Exception {
// lookup)
assertLogCount(collectionClusterStateLogs, 1);
// 1 call to fetch entire cluster state from HttpCSP.fetchLiveNodes()
- assertLogCount(entireClusterStateLogs, 1);
+ assertLogCount(entireClusterStateLogs, 0);
Review Comment:
Awesome.
At this juncture, I recommend changes to this test because the way it works
now is awkward: Only have one LogListener for the "adminRequestLogs" (simple;
3 is more confusing). Consume the log messages in the test (don't merely
count) to assert that the log message is what it should be, ideally with a
substring check or a simple regex if you must. Consume & assert this after
each step that we know will emit a request: after client construction and once
more after the first "add" (doc). Your code comments here are then redundant
as the assertions will be more specific at the right times to be self
explanatory.
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##
@@ -100,7 +100,13 @@ public class CloudHttp2SolrClientTest extends
SolrCloudTestCase {
private static final Pattern PATTERN_WITHOUT_COLLECTION =
Pattern.compile(
"path=/admin/collections.*params=\\{[^}]*action=CLUSTERSTATUS"
- + "(?![^}]*collection=)[^}]*\\}");
+ + "(?=[^}]*includeAll=true)"
Review Comment:
let's do away with this regex; it's gotten out of control and should cause
us to re-think what we're doing (see my other comment)
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##
@@ -84,6 +84,11 @@ public void init(List solrUrls) throws Exception {
/** Create a SolrClient implementation that uses the specified Solr node URL
*/
protected abstract SolrClient getSolrClient(String baseUrl);
+ @Override
Review Comment:
minor: I meant only a code comment and not javadocs. Javadocs are to define
the purpose of the method; the ~first comment inside a method (or class when
appropriate) is for implementation notes/details like this
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -191,43 +234,23 @@ public void getClusterStatus(NamedList results)
}
String configName = clusterStateCollection.getConfigName();
collectionStatus.put("configName", configName);
- if (message.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
+ if (solrParams.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
PerReplicaStates prs = clusterStateCollection.getPerReplicaStates();
collectionStatus.put("PRS", prs);
}
collectionProps.add(name, collectionStatus);
}
-List liveNodes =
-
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
-
-// now we need to walk the collectionProps tree to cross-check replica
state with live nodes
-crossCheckReplicaStateWithLiveNodes(liveNodes, collectionProps);
-
-NamedList clusterStatus = new SimpleOrderedMap<>();
-clusterStatus.add("collections", collectionProps);
-
-// read cluster properties
-Map clusterProps = zkStateReader.getClusterProperties();
-if (clusterProps != null && !clusterProps.isEmpty()) {
- clusterStatus.add("properties", clusterProps);
-}
-
// add the alias map too
Map collectionAliasMap = aliases.getCollectionAliasMap();
// comma delim
if (!collectionAliasMap.isEmpty()) {
clusterStatus.add("aliases", collectionAliasMap);
Review Comment:
the concept of `includeAll=false` is that you're then specifying what
sections you want and only getting those, not other stuff. "aliases" is a
top-level response section that doesn't yet have a boolean to ask for it.
Wether it's cheap or not (what you imply?) shouldn't matter IMO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17102: Replaced VersionBucket array with locks on-demand [solr]
dsmiley merged PR #2548: URL: https://github.com/apache/solr/pull/2548 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1706190275
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -89,14 +93,61 @@ public static Health combine(Collection states) {
}
}
- public ClusterStatus(ZkStateReader zkStateReader, ZkNodeProps props) {
+ public ClusterStatus(ZkStateReader zkStateReader, SolrParams params) {
this.zkStateReader = zkStateReader;
-this.message = props;
-collection = props.getStr(ZkStateReader.COLLECTION_PROP);
+this.solrParams = params;
+collection = params.get(ZkStateReader.COLLECTION_PROP);
}
public void getClusterStatus(NamedList results)
throws KeeperException, InterruptedException {
+NamedList clusterStatus = new SimpleOrderedMap<>();
+
+boolean includeAll = solrParams.getBool(INCLUDE_ALL, true);
+boolean withLiveNodes = solrParams.getBool(LIVENODES_PROP, includeAll);
+boolean withClusterProperties = solrParams.getBool(CLUSTER_PROP,
includeAll);
+boolean withRoles = solrParams.getBool(ZkStateReader.ROLES_PROP,
includeAll);
+boolean withCollection = includeAll || (collection != null);
+
+List liveNodes = null;
+if (withLiveNodes || collection != null) {
+ liveNodes =
+
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
+ // add live_nodes
+ if (withLiveNodes) clusterStatus.add("live_nodes", liveNodes);
+}
+if (withCollection) {
+ assert liveNodes != null;
+ fetchClusterStatusForCollOrAlias(clusterStatus, liveNodes);
+}
+
+if (withClusterProperties) {
+ Map clusterProps = Collections.emptyMap();
+ // read cluster properties
+ clusterProps = zkStateReader.getClusterProperties();
+ if (clusterProps == null || clusterProps.isEmpty()) {
Review Comment:
You are right. Updated.
##
solr/solr-ref-guide/modules/deployment-guide/pages/cluster-node-management.adoc:
##
@@ -99,6 +99,41 @@ Multiple shard names can be specified as a comma-separated
list.
+
This can be used if you need the details of the shard where a particular
document belongs to and you don't know which shard it falls under.
+`liveNodes`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: will default to the default value of `includeAll`
parameter specified below
+|===
++
+If set to true, returns the status of live nodes in the cluster.
+
+`clusterProperties`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: will default to the default value of `includeAll`
parameter specified below
+|===
++
+If set to true, returns the properties of the cluster.
+
+`roles`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: will default to the default value of `includeAll`
parameter specified below
+|===
++
+If set to true, returns the roles within the cluster.
+
+`includeAll`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: true
+|===
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-13350, SOLR-17298: use multiThreaded=false default; document multiThreaded parameter; [solr]
gus-asf merged PR #2596: URL: https://github.com/apache/solr/pull/2596 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1706167903
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -191,43 +234,23 @@ public void getClusterStatus(NamedList results)
}
String configName = clusterStateCollection.getConfigName();
collectionStatus.put("configName", configName);
- if (message.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
+ if (solrParams.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
PerReplicaStates prs = clusterStateCollection.getPerReplicaStates();
collectionStatus.put("PRS", prs);
}
collectionProps.add(name, collectionStatus);
}
-List liveNodes =
-
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
-
-// now we need to walk the collectionProps tree to cross-check replica
state with live nodes
-crossCheckReplicaStateWithLiveNodes(liveNodes, collectionProps);
-
-NamedList clusterStatus = new SimpleOrderedMap<>();
-clusterStatus.add("collections", collectionProps);
-
-// read cluster properties
-Map clusterProps = zkStateReader.getClusterProperties();
-if (clusterProps != null && !clusterProps.isEmpty()) {
- clusterStatus.add("properties", clusterProps);
-}
-
// add the alias map too
Map collectionAliasMap = aliases.getCollectionAliasMap();
// comma delim
if (!collectionAliasMap.isEmpty()) {
clusterStatus.add("aliases", collectionAliasMap);
Review Comment:
Not sure. It is a simple fetch from another map (does list to comma
delimited conversion). Not sure the caller would want to know about aliases too
in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1706145292
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -89,14 +93,61 @@ public static Health combine(Collection states) {
}
}
- public ClusterStatus(ZkStateReader zkStateReader, ZkNodeProps props) {
+ public ClusterStatus(ZkStateReader zkStateReader, SolrParams params) {
this.zkStateReader = zkStateReader;
-this.message = props;
-collection = props.getStr(ZkStateReader.COLLECTION_PROP);
+this.solrParams = params;
+collection = params.get(ZkStateReader.COLLECTION_PROP);
Review Comment:
other params except `collection` are used only in `getClusterStatus()`, but
collection is also used in `fetchClusterStatusForCollOrAlias()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[PR] SOLR-17249 Edit .asf.yaml on main branch [solr]
janhoy opened a new pull request, #2624: URL: https://github.com/apache/solr/pull/2624 Not tested but I think it should do the trick. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] SOLR-17253 Release wizard suggests wrong minor version for jira [solr]
janhoy opened a new pull request, #2622: URL: https://github.com/apache/solr/pull/2622 This should do the trick for suggesting correct jira version -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] SOLR-17394: Check response status in IndexFetcher [solr]
gerlowskija opened a new pull request, #2621: URL: https://github.com/apache/solr/pull/2621 https://issues.apache.org/jira/browse/SOLR-17394 # Description Prior to this commit, IndexFetcher would mis-read 404 responses from other nodes. It eventually catches this mistake and retries, but not before it wastefully allocates a massive (> 1GB) byte array. These allocations were relatively infrequent, as they require out of date leader information to occur. But they are big enough to impact performance in production, and were observed to cause OOM's when running tests. # Solution This commit avoids this by checking the HTTP status code of all IndexFetcher requests/responses, even those that utilize InputStreamResponseParser. # Tests I don't have unit tests for this as it's a bit hard to exercise in isolation. But I am able to see the newly-introduced IOE message in the logs, and the massive `byte[]` is no longer allocated. The other way I've tested this, such as it is, is through 'beasting' a particular test which trigger a OOM and heap dump on about every 1/10 iterations. Setting `tests.heapsize` to any moderate value (e.g. `1GB`) in `gradle.properties` and running the command below reproduces the OOM reliably for me on `main` without this PR. (And reproduces the OOM never with the PR in place.) ``` ./gradlew -p solr/solrj beast -Ptests.seed=F024B5067D7FCE8F -Ptests.neverUpToDate=true -Ptests.dups=50 --tests CloudHttp2SolrClientTest ``` # Checklist Please review the following and check all that apply: - [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability. - [x] I have created a Jira issue and added the issue ID to my pull request title. - [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended) - [x] I have developed this patch against the `main` branch. - [x] I have run `./gradlew check`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
dsmiley commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1706026056
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -89,14 +93,61 @@ public static Health combine(Collection states) {
}
}
- public ClusterStatus(ZkStateReader zkStateReader, ZkNodeProps props) {
+ public ClusterStatus(ZkStateReader zkStateReader, SolrParams params) {
this.zkStateReader = zkStateReader;
-this.message = props;
-collection = props.getStr(ZkStateReader.COLLECTION_PROP);
+this.solrParams = params;
+collection = params.get(ZkStateReader.COLLECTION_PROP);
}
public void getClusterStatus(NamedList results)
throws KeeperException, InterruptedException {
+NamedList clusterStatus = new SimpleOrderedMap<>();
+
+boolean includeAll = solrParams.getBool(INCLUDE_ALL, true);
+boolean withLiveNodes = solrParams.getBool(LIVENODES_PROP, includeAll);
+boolean withClusterProperties = solrParams.getBool(CLUSTER_PROP,
includeAll);
+boolean withRoles = solrParams.getBool(ZkStateReader.ROLES_PROP,
includeAll);
+boolean withCollection = includeAll || (collection != null);
+
+List liveNodes = null;
+if (withLiveNodes || collection != null) {
+ liveNodes =
+
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
+ // add live_nodes
+ if (withLiveNodes) clusterStatus.add("live_nodes", liveNodes);
+}
+if (withCollection) {
+ assert liveNodes != null;
+ fetchClusterStatusForCollOrAlias(clusterStatus, liveNodes);
+}
+
+if (withClusterProperties) {
+ Map clusterProps = Collections.emptyMap();
+ // read cluster properties
+ clusterProps = zkStateReader.getClusterProperties();
+ if (clusterProps == null || clusterProps.isEmpty()) {
Review Comment:
isEmpty check isn't accomplishing anything here
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -89,14 +93,61 @@ public static Health combine(Collection states) {
}
}
- public ClusterStatus(ZkStateReader zkStateReader, ZkNodeProps props) {
+ public ClusterStatus(ZkStateReader zkStateReader, SolrParams params) {
this.zkStateReader = zkStateReader;
-this.message = props;
-collection = props.getStr(ZkStateReader.COLLECTION_PROP);
+this.solrParams = params;
+collection = params.get(ZkStateReader.COLLECTION_PROP);
Review Comment:
isn't it a little awkward that this param is initialized here despite lots
of params being taken in getClusterStatus? (change or not as you wish)
##
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##
@@ -191,43 +234,23 @@ public void getClusterStatus(NamedList results)
}
String configName = clusterStateCollection.getConfigName();
collectionStatus.put("configName", configName);
- if (message.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
+ if (solrParams.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
PerReplicaStates prs = clusterStateCollection.getPerReplicaStates();
collectionStatus.put("PRS", prs);
}
collectionProps.add(name, collectionStatus);
}
-List liveNodes =
-
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
-
-// now we need to walk the collectionProps tree to cross-check replica
state with live nodes
-crossCheckReplicaStateWithLiveNodes(liveNodes, collectionProps);
-
-NamedList clusterStatus = new SimpleOrderedMap<>();
-clusterStatus.add("collections", collectionProps);
-
-// read cluster properties
-Map clusterProps = zkStateReader.getClusterProperties();
-if (clusterProps != null && !clusterProps.isEmpty()) {
- clusterStatus.add("properties", clusterProps);
-}
-
// add the alias map too
Map collectionAliasMap = aliases.getCollectionAliasMap();
// comma delim
if (!collectionAliasMap.isEmpty()) {
clusterStatus.add("aliases", collectionAliasMap);
Review Comment:
oh wait; should there be withAliases ?
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##
@@ -84,6 +84,11 @@ public void init(List solrUrls) throws Exception {
/** Create a SolrClient implementation that uses the specified Solr node URL
*/
protected abstract SolrClient getSolrClient(String baseUrl);
+ @Override
Review Comment:
this would be a good code comment
##
solr/solr-ref-guide/modules/deployment-guide/pages/cluster-node-management.adoc:
##
@@ -99,6 +99,41 @@ Multiple shard names can be specified as a comma-separated
list.
+
This can be used if you need the details of the shard where a particular
document belongs to and you don't know which shard it falls under.
+`liveNodes`::
++
Re: [PR] SOLR-17391: Cached thread pools actually have more than one thread. [solr]
dsmiley commented on code in PR #2619:
URL: https://github.com/apache/solr/pull/2619#discussion_r1705975931
##
solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java:
##
@@ -243,15 +243,27 @@ public static ExecutorService
newMDCAwareCachedThreadPool(ThreadFactory threadFa
0, Integer.MAX_VALUE, 60L, TimeUnit.SECONDS, new SynchronousQueue<>(),
threadFactory);
}
+ /**
Review Comment:
Suggested:
> Create a new pool of threads. Threads are created for new work if there
is room to do so up to {@code maxThreads}. Beyond that, the queue is used up
to {@code queueCapacity}. Beyond that, work is rejected with an exception.
Unused threads will be closed after 60 seconds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reduce thread contention in ZkStateReader.getCollectionProperties() [solr]
aparnasuresh85 commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1706005573
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -300,10 +313,11 @@ private VersionedCollectionProps
fetchCollectionProperties(String collection, Wa
throws KeeperException, InterruptedException {
final String znodePath = getCollectionPropsPath(collection);
// lazy init cache cleaner once we know someone is using collection
properties.
-if (collectionPropsCacheCleaner == null) {
- synchronized (zkStateReader.getUpdateLock()) { // Double-checked locking
-if (collectionPropsCacheCleaner == null) {
- collectionPropsCacheCleaner = notifications.submit(new
CacheCleaner());
+if (cacheCleanerThread == null) {
+ synchronized (this) {
+if (cacheCleanerThread == null) {
Review Comment:
Can we instead revert to the logic 2 commits prior that used the
ExecutorService and the Atomic Boolean to ensure the `cacheCleaner` is
initialized only once?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1705971067
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##
@@ -84,6 +84,11 @@ public void init(List solrUrls) throws Exception {
/** Create a SolrClient implementation that uses the specified Solr node URL
*/
protected abstract SolrClient getSolrClient(String baseUrl);
+ @Override
Review Comment:
Super class ClusterStateProvider's implementation calls
`getClusterState().getCollectionOrNull(collectionName) `- in that case,
`BaseHttpCSP` could make a call to fetch the entire cluster state! This change
is to prevent that from happening.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1705971067
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##
@@ -84,6 +84,11 @@ public void init(List solrUrls) throws Exception {
/** Create a SolrClient implementation that uses the specified Solr node URL
*/
protected abstract SolrClient getSolrClient(String baseUrl);
+ @Override
Review Comment:
Super class ClusterStateProvider's implementation calls
getClusterState().getCollectionOrNull(collectionName) - in that case
BaseHttpCSP could make a call to fetch the entire cluster state! This change is
to prevent that from happening.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1705968724
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##
@@ -100,7 +100,13 @@ public class CloudHttp2SolrClientTest extends
SolrCloudTestCase {
private static final Pattern PATTERN_WITHOUT_COLLECTION =
Pattern.compile(
"path=/admin/collections.*params=\\{[^}]*action=CLUSTERSTATUS"
- + "(?![^}]*collection=)[^}]*\\}");
+ + "(?=[^}]*includeAll=true)"
Review Comment:
Regex pattern (sad) updated to reflect the new params to CLUSTERSTATUS call
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
aparnasuresh85 commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1705968302
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##
@@ -291,7 +297,7 @@ public void testHttpCspPerf() throws Exception {
// lookup)
assertLogCount(collectionClusterStateLogs, 1);
// 1 call to fetch entire cluster state from HttpCSP.fetchLiveNodes()
- assertLogCount(entireClusterStateLogs, 1);
+ assertLogCount(entireClusterStateLogs, 0);
Review Comment:
No more calls to fetch entire cluster state!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] [SOLR-10654] Rename instances of SolrPrometheusExporter to SolrPrometheusFormatter [solr]
mlbiscoc commented on PR #2620: URL: https://github.com/apache/solr/pull/2620#issuecomment-2271654565 @dsmiley Luckily Intellij makes everything relatively easy to rename. Renamed everything from `exporter` to `formatter`. I personally like `formatter` better than your recommendation of `format` but can change again if you prefer. Only thing I kept as `export` was the export functions such as `exportGauge()` functions. If you prefer to I change those to `formartGauge()`, I can change those as well. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] [SOLR-10654] Rename instances of SolrPrometheusExporter to SolrPrometheusFormatter [solr]
mlbiscoc opened a new pull request, #2620: URL: https://github.com/apache/solr/pull/2620 https://issues.apache.org/jira/browse/SOLR-10654 # Description Internal `SolrPrometheusExporter` names was confusing with existing `Prometheus Exporter`. # Solution Rename all instances of `SolrPrometheusExporter` to `SolrPrometheusFormatter` including class names, functions and parameters. # Tests Renamed test names as well. # Checklist Please review the following and check all that apply: - [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability. - [ ] I have created a Jira issue and added the issue ID to my pull request title. - [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended) - [ ] I have developed this patch against the `main` branch. - [ ] I have run `./gradlew check`. - [ ] I have added tests for my changes. - [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17391: Cached thread pools actually have more than one thread. [solr]
madrob commented on code in PR #2619:
URL: https://github.com/apache/solr/pull/2619#discussion_r1705773515
##
solr/solrj/src/test/org/apache/solr/common/util/ExecutorUtilTest.java:
##
@@ -108,6 +111,48 @@ public void
testExecutorUtilAwaitsTerminationWhenTaskRespectsInterupt() throws E
}
}
+ @Test
+ public void testCMDCAwareCachedThreadPool() {
+// 5 threads max, unbounded queue
+ExecutorService executor =
+ExecutorUtil.newMDCAwareCachedThreadPool(
+5, Integer.MAX_VALUE, new NamedThreadFactory("test"));
+
+AtomicInteger concurrentTasks = new AtomicInteger();
+AtomicInteger maxConcurrentTasks = new AtomicInteger();
+int taskCount = random().nextInt(100);
+
+for (int i = 0; i < taskCount; i++) {
+ String core = "id_" + random().nextLong();
+
+ Callable task =
+ () -> {
+// ensure we never have too many concurrent tasks
+int concurrent = concurrentTasks.incrementAndGet();
+assertTrue(concurrent <= 5);
+maxConcurrentTasks.getAndAccumulate(concurrent, Math::max);
+
+// assert MDC context is copied from the parent thread that
submitted the task
+assertEquals(core, MDC.get("core"));
+
+// Sleep for a couple of millis before considering the task is done
+int delay = random().nextInt(10);
+Thread.sleep(delay);
+concurrentTasks.decrementAndGet();
+return null;
+ };
+
+ MDCLoggingContext.setCoreName(core);
+ executor.submit(task);
+}
+
+ExecutorUtil.shutdownAndAwaitTermination(executor);
+
+// assert the pool was actually multithreaded. Since we submitted many
tasks,
+// all the threads should have been started
+assertEquals(5, maxConcurrentTasks.get());
Review Comment:
Your previous code submitted uniformly random 0-100 tasks, it would have
failed 5% of the time. Thank you for adding the `5 + ...` and the latch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17391: Cached thread pools actually have more than one thread. [solr]
psalagnac commented on code in PR #2619:
URL: https://github.com/apache/solr/pull/2619#discussion_r1705765822
##
solr/solrj/src/test/org/apache/solr/common/util/ExecutorUtilTest.java:
##
@@ -108,6 +111,48 @@ public void
testExecutorUtilAwaitsTerminationWhenTaskRespectsInterupt() throws E
}
}
+ @Test
+ public void testCMDCAwareCachedThreadPool() {
+// 5 threads max, unbounded queue
+ExecutorService executor =
+ExecutorUtil.newMDCAwareCachedThreadPool(
+5, Integer.MAX_VALUE, new NamedThreadFactory("test"));
+
+AtomicInteger concurrentTasks = new AtomicInteger();
+AtomicInteger maxConcurrentTasks = new AtomicInteger();
+int taskCount = random().nextInt(100);
+
+for (int i = 0; i < taskCount; i++) {
+ String core = "id_" + random().nextLong();
+
+ Callable task =
+ () -> {
+// ensure we never have too many concurrent tasks
+int concurrent = concurrentTasks.incrementAndGet();
+assertTrue(concurrent <= 5);
+maxConcurrentTasks.getAndAccumulate(concurrent, Math::max);
+
+// assert MDC context is copied from the parent thread that
submitted the task
+assertEquals(core, MDC.get("core"));
+
+// Sleep for a couple of millis before considering the task is done
+int delay = random().nextInt(10);
+Thread.sleep(delay);
+concurrentTasks.decrementAndGet();
+return null;
+ };
+
+ MDCLoggingContext.setCoreName(core);
+ executor.submit(task);
+}
+
+ExecutorUtil.shutdownAndAwaitTermination(executor);
+
+// assert the pool was actually multithreaded. Since we submitted many
tasks,
+// all the threads should have been started
+assertEquals(5, maxConcurrentTasks.get());
Review Comment:
I added a latch in the test, so we don't depend on the scheduler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17391: Cached thread pools actually have more than one thread. [solr]
psalagnac commented on code in PR #2619:
URL: https://github.com/apache/solr/pull/2619#discussion_r1705724005
##
solr/solrj/src/test/org/apache/solr/common/util/ExecutorUtilTest.java:
##
@@ -108,6 +111,48 @@ public void
testExecutorUtilAwaitsTerminationWhenTaskRespectsInterupt() throws E
}
}
+ @Test
+ public void testCMDCAwareCachedThreadPool() {
+// 5 threads max, unbounded queue
+ExecutorService executor =
+ExecutorUtil.newMDCAwareCachedThreadPool(
+5, Integer.MAX_VALUE, new NamedThreadFactory("test"));
+
+AtomicInteger concurrentTasks = new AtomicInteger();
+AtomicInteger maxConcurrentTasks = new AtomicInteger();
+int taskCount = random().nextInt(100);
+
+for (int i = 0; i < taskCount; i++) {
+ String core = "id_" + random().nextLong();
+
+ Callable task =
+ () -> {
+// ensure we never have too many concurrent tasks
+int concurrent = concurrentTasks.incrementAndGet();
+assertTrue(concurrent <= 5);
+maxConcurrentTasks.getAndAccumulate(concurrent, Math::max);
+
+// assert MDC context is copied from the parent thread that
submitted the task
+assertEquals(core, MDC.get("core"));
+
+// Sleep for a couple of millis before considering the task is done
+int delay = random().nextInt(10);
+Thread.sleep(delay);
+concurrentTasks.decrementAndGet();
+return null;
+ };
+
+ MDCLoggingContext.setCoreName(core);
+ executor.submit(task);
+}
+
+ExecutorUtil.shutdownAndAwaitTermination(executor);
+
+// assert the pool was actually multithreaded. Since we submitted many
tasks,
+// all the threads should have been started
+assertEquals(5, maxConcurrentTasks.get());
Review Comment:
Theoretically yes. But that's very unlikely as we submit 1000 tasks.
I consider a decent implementation of the thread pool should always start
the 5 requested threads.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17391: Cached thread pools actually have more than one thread. [solr]
madrob commented on code in PR #2619:
URL: https://github.com/apache/solr/pull/2619#discussion_r1705719172
##
solr/solrj/src/test/org/apache/solr/common/util/ExecutorUtilTest.java:
##
@@ -108,6 +111,48 @@ public void
testExecutorUtilAwaitsTerminationWhenTaskRespectsInterupt() throws E
}
}
+ @Test
+ public void testCMDCAwareCachedThreadPool() {
+// 5 threads max, unbounded queue
+ExecutorService executor =
+ExecutorUtil.newMDCAwareCachedThreadPool(
+5, Integer.MAX_VALUE, new NamedThreadFactory("test"));
+
+AtomicInteger concurrentTasks = new AtomicInteger();
+AtomicInteger maxConcurrentTasks = new AtomicInteger();
+int taskCount = random().nextInt(100);
+
+for (int i = 0; i < taskCount; i++) {
+ String core = "id_" + random().nextLong();
+
+ Callable task =
+ () -> {
+// ensure we never have too many concurrent tasks
+int concurrent = concurrentTasks.incrementAndGet();
+assertTrue(concurrent <= 5);
+maxConcurrentTasks.getAndAccumulate(concurrent, Math::max);
+
+// assert MDC context is copied from the parent thread that
submitted the task
+assertEquals(core, MDC.get("core"));
+
+// Sleep for a couple of millis before considering the task is done
+int delay = random().nextInt(10);
+Thread.sleep(delay);
+concurrentTasks.decrementAndGet();
+return null;
+ };
+
+ MDCLoggingContext.setCoreName(core);
+ executor.submit(task);
+}
+
+ExecutorUtil.shutdownAndAwaitTermination(executor);
+
+// assert the pool was actually multithreaded. Since we submitted many
tasks,
+// all the threads should have been started
+assertEquals(5, maxConcurrentTasks.get());
Review Comment:
It's possible that taskCount will be < 5 and the test will fail 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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17381: Make CLUSTERSTATUS configurable [solr]
dsmiley commented on PR #2599: URL: https://github.com/apache/solr/pull/2599#issuecomment-2271538712 @gerlowskija in V2, I'm thinking these are separate endpoints -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] SOLR-17391: Cached thread pools actually have more than one thread. [solr]
psalagnac opened a new pull request, #2619: URL: https://github.com/apache/solr/pull/2619 https://issues.apache.org/jira/browse/SOLR-17391 # Description Cached thread pools never start more than one thread. Consequently, all the async tasks are executed sequentially. This is mostly a performance issue of backup/restore with large collections. # Solution Fix `newMDCAwareCachedThreadPool` so it behaves as expected. # Tests Added a test to assert the pool is multithreaded. # Checklist Please review the following and check all that apply: - [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability. - [x] I have created a Jira issue and added the issue ID to my pull request title. - [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended) - [x] I have developed this patch against the `main` branch. - [x] I have run `./gradlew check`. - [x] I have added tests for my changes. - [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] SOLR-16824: Backport single dash arguments migration [solr]
malliaridis opened a new pull request, #2618: URL: https://github.com/apache/solr/pull/2618 https://issues.apache.org/jira/browse/SOLR-16824 # Description Various code fragments are only available in `branch_9X` and require separate migration. # Solution This PR addresses various argument migrations that either are not in `main` branch or break backwards / forwards compatibility. The changes are split in multiple commits that may be cherry-picked to apply to `main`. Changes may also be squashed before merging if they are not cherry-picked. Open tasks for this PR: - [ ] Check and fix arguments in CLI classes for full support - [ ] Check and fix deprecation messages from arguments in CLI classes - [ ] Add tests for deprecated arguments to ensure compatibility # Tests _This migration does migrate test arguments too and therefore tests that are ensuring backwards compatibility are not explcitily present._ # Checklist Please review the following and check all that apply: - [X] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability. - [ ] I have created a Jira issue and added the issue ID to my pull request title. - [X] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended) - [ ] I have developed this patch against the `main` branch. - [ ] I have run `./gradlew check`. - [ ] I have added tests for my changes. - [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
