Repository navigation
Conversation
rzo1
reviewed
Oct 18, 2026
rzo1
left a comment
rzo1
left a comment
Contributor
There was a problem hiding this comment.
LGTM.
A few things before we merge:
- Rebase merge is enabled in
.asf.yaml, and the commit "replace the legacy opensearch sources..." doesn't compile without the one after it. Let's squash-merge this one. Your note aboutgit blameassumes that anyway. - Setting
opensearch.<type>.bulkActions: -1now makes the bolt fail at startup, because the value ends up innew ArrayList<>(bulkActions). The legacy module treated -1 as "no count limit". Could you either keep that behaviour for values ≤ 0, or mention it in the upgrade section of the README? warnOnRemovedKeysonly looks atopensearch.<type>.sniffandopensearch.<type>.responseBufferSize. If someone has the globalopensearch.snifforopensearch.responseBufferSize, they're ignored without a warning.- The tests now run against OpenSearch 3.9.0, but the archetype's
docker-compose.ymlstill uses 2.19.4 for both opensearch and opensearch-dashboards. Can you bump those to 3.9.0 as well, so new projects start on the version we actually test with? - As discussed in #2212, please add a short migration note to the PR description itself: switch the dependency to
stormcrawler-opensearch, removesniffandresponseBufferSize, list every node inaddresses, and move custom code to the new client types. The README link is fine, but the PR description is where people tend to look later.
Contributor
Author
Contributor
|
great work @dpol1 A missing mapping file now logs a WARN and the component starts without creating its index, where it used to fail at startup. This is clearly documented but failing fast seemed safer to me. Another thing well documented issue is the possible performance hit for HybridSpouts with the new implementation. You can find a ndjson file in https://nightlies.apache.org/stormcrawler/ |
jnioche
force-pushed
the
opensearch-3.x
branch
from
October 20, 2026 15:46
9390022 to
bb2fe2c
Compare
Metadata.setValue(key, null) stores a null element, and addValue keeps it next to the new value. StatusUpdaterBolt copied the values of every metadata key with List.of, and IndexerBolt the values of the keys with more than one value. List.of throws a NullPointerException on a null element; the bolts caught it and failed the tuple. Both bolts now copy the values with Arrays.asList, and the null is written as a JSON null inside the array, as the legacy module does. Signed-off-by: Davide Polato <dpol1@apache.org>
The files of external/opensearch/src that also exist in external/opensearch-java take the content of their opensearch-java counterparts in place, so git blame keeps the lines that did not change. The files that exist only in external/opensearch-java are moved. The following commits update the module's POM, configuration and README. Signed-off-by: Davide Polato <dpol1@apache.org>
The module depends on opensearch-java 3.10.0 instead of the high-level REST client and the sniffer, and its tests run against OpenSearch 3.5.0. The artifactId stays stormcrawler-opensearch. external/opensearch-java leaves the reactor; its README is merged into the one of external/opensearch in a later commit. Signed-off-by: Davide Polato <dpol1@apache.org>
The opensearch-java client has no sniffer and no per-request response buffer setting. The keys are removed from the module's opensearch-conf.yaml, from the one of the archetype and from the configuration reference. When they are set, OpenSearchConnection still logs a WARN, which now points to external/opensearch/README.md. The configuration reference and the Javadoc of DelegateRefresher no longer mention a second OpenSearch module. Signed-off-by: Davide Polato <dpol1@apache.org>
The README of external/opensearch describes the client and AsyncBulkProcessor, and keeps the archetype and dashboard instructions. A new section lists what changes for users of earlier versions: the configuration keys, the behaviour differences and the types that custom code has to use. Signed-off-by: Davide Polato <dpol1@apache.org>
The legacy module tested that a running client sends the credentials to a configured address and not to another one, that credentials need both a user and a password, that host and scheme are matched without regard to case, and that WaitAckCache.shutdown() fails the tuples still waiting. The opensearch-java tests only checked how the credential scopes are built. The tests are adapted to HttpClient 5 and the opensearch-java transport. Two legacy tests are not carried over: the sniffer scheme test, since there is no sniffer, and the test for a credentials lookup without an origin, since the transport's preemptive Basic scheme looks the credentials up with the host of the request. Signed-off-by: Davide Polato <dpol1@apache.org>
With opensearch.disable.tls.validation set, OpenSearchConnection replaces the connection manager of the transport to install a trust-all TLS strategy. The replacement had no pool limits, so HttpClient 5 allowed 5 connections per node and 25 in total, instead of the 10 and 30 that the transport sets. The replacement now uses the transport's limits. The new test sends 12 requests to a local server that never answers and checks that 10 connections are open. Signed-off-by: Davide Polato <dpol1@apache.org>
Signed-off-by: Davide Polato <dpol1@apache.org>
The legacy BulkProcessor treats bulkActions -1 as no limit on the number of operations. AsyncBulkProcessor used the value as the capacity of its buffer, so -1 made the bolt fail at startup. A negative value now turns off the count limit, while the size limit and the flush interval still apply. 0 keeps sending each operation in its own bulk, as in both modules. Signed-off-by: Davide Polato <dpol1@apache.org>
OpenSearchConnection logged a WARN only for the per-type keys, so a global opensearch.sniff or opensearch.responseBufferSize was ignored without a warning. The WARN now covers both forms and names the key that is set. Signed-off-by: Davide Polato <dpol1@apache.org>
The module tests run against OpenSearch 3.9.0, while the archetype's compose file still started 2.19.4 for OpenSearch and Dashboards. AbstractOpenSearchTest now also falls back to 3.9.0 when the opensearch-version property is not set, as in a run from an IDE. Signed-off-by: Davide Polato <dpol1@apache.org>
IndexCreation logged a WARN when the <boltType>.mapping resource was not on the classpath, and the component started without its index or template. It now looks the resource up with Resources.getResource before the try, which throws IllegalArgumentException naming the resource, so the bolt, spout or metrics consumer fails in prepare() or open(). The README no longer lists the WARN among the changes in behaviour. Signed-off-by: Davide Polato <dpol1@apache.org>
jnioche
force-pushed
the
opensearch-3.x
branch
from
October 21, 2026 04:41
bb2fe2c to
3cb5850
Compare
createIndex and createTemplate returned false when OpenSearch answered the creation with an error status, and checkOrCreateIndex only logged that the index was not created. They now log a WARN with the status and the response body, as the legacy module did with the exception of the high-level client. The comments that said they only log exceptions are removed. IndexCreationTest checks both paths with a mocked 400.
Contributor
Author
|
thanks @jnioche 👍 pushed: a component that has to create its index or template fails at startup when the mapping resource is missing, as 3.x did. When OpenSearch rejects the creation, benchmarks on OpenSearch 3.9.0, main vs this branch, median and range of 3 runs:
on writes the branch is about 4% lower in these runs. in an earlier series it was 4% higher. the setup, the input and the read settings are in the description. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second part of #2212. As @jnioche suggested, the opensearch-java code moves into
external/opensearch, so the artifactstormcrawler-opensearch, its archetype, the configuration files and the dashboards stay where they are.external/opensearch-javais removed.The files present in both modules are overwritten in place, so that
git blamekeeps the legacy history. The ones only in opensearch-java (AsyncBulkProcessorand a few tests) are moved withgit mv. This needs a squash merge, since the commit that replaces the sources doesn't compile on its own.Migration
Users of
stormcrawler-opensearch-javaswitch the dependency tostormcrawler-opensearch. Packages, classes and configuration keys are the same.Users of
stormcrawler-opensearch3.x:opensearch.sniff,opensearch.responseBufferSizeand their per-type formsopensearch.<type>.addresses, or put a load balancer in front of the cluster, as there is no sniffing anymoreOpenSearchClientandBulkOperationexternal/opensearch/README.mdlists the other differences under "Upgrading from versions before 4.0".Other changes
StatusUpdaterBoltandIndexerBoltcould fail the tuple on a null metadata value, because ofList.of. They now useArrays.asListand write the null, as the legacy module does (NullMetadataValueTest).IndexCreationlogs the status and the response body at WARN. opensearch-java only loggedcreated? falseat INFO.opensearch.<type>.bulkActionsturns off the count limit, as -1 did in the legacy module. In opensearch-java, -1 made the bolt fail at startup.sniffandresponseBufferSizekeys covers their global forms too.docker-compose.ymlruns OpenSearch and OpenSearch Dashboards 3.9.0, the version the module tests use.opensearch.disable.tls.validationthe connection pool fell back to the HttpClient 5 defaults, 5 connections per node and 25 in total. It now uses the transport's 10 and 30.WaitAckCache.shutdown()and of which hosts get the credentials.Testing
The module tests pass locally with Docker. I also crawled a small local site for 3 minutes with a project generated from the archetype, on OpenSearch 2.19.4: pages fetched and indexed, metrics written, no mapping or query errors.
The benchmark numbers are in this comment, and the ranges overlap with main's on all three benchmarks. They ran on OpenSearch 3.9.0 with one instance of each component, on 2M shuffled lines from
outlinks.ndjson.gz. For the reads I gave theknownlines a pastrefetchable_from_dateand usedmax.buckets: 500,max.urls.per.bucket: 10andflushInterval: 1son both sides, because the archetype values keep the spouts at about 5 URLs/s.