github-actions[bot] commented on code in PR #67352:
URL: https://github.com/apache/doris/pull/67352#discussion_r3914223037
##########
contrib/clucene:
##########
@@ -1 +1 @@
-Subproject commit 08d45e541fef1e48dc4ab00d2d5fde9569b49ee3
+Subproject commit abe2b71a1c1afd3120ce39740d2346d07a5c2309
Review Comment:
**[P1] Keep the embedded-NUL crash fix in the CLucene pin**
This moves the submodule backward: `abe2b71a1c1` is the direct parent of the
base pin `08d45e541fe`. Doris commit `e59320b9571f1ac7796ad6562097730e280b3b60`
deliberately advanced this exact pin after reproducing BE SIGSEGV/restart loops
for indexed strings containing an embedded NUL; the dropped commit adds the
`SDocumentWriter::addPosition` guard that turns that input into a CLucene
error. The current Doris writer still forwards each `Slice` pointer and
explicit length into CLucene without another embedded-NUL rejection, so
rebuilding this head restores the crash path. Please retain `08d45e541fe` (or a
descendant containing the same guard).
##########
be/test/udf/python/python_server_test.cpp:
##########
@@ -178,6 +158,32 @@ class PythonServerTest : public ::testing::Test {
return python_path;
}
+ std::string create_fake_python_waiting_for_peer(const std::string&
binary_name,
+ const std::string& version,
+ const std::string&
own_marker,
+ const std::string&
peer_marker) {
+ std::string bin_dir = test_dir_ + "/bin";
+ std::string python_path = bin_dir + "/" + binary_name;
+ fs::create_directories(bin_dir);
+
+ std::ofstream ofs(python_path);
+ ofs << "#!/bin/bash\n";
+ ofs << "if [ \"$1\" = \"--version\" ]; then echo 'Python " << version
<< "'; exit 0; fi\n";
+ ofs << "if [ ! -f \"$2\" ]; then exit 2; fi\n";
+ ofs << "touch \"" << own_marker << "\"\n";
+ // Each version must enter fork() before either child exposes its
socket. A manager-wide
+ // initialization lock makes the first child time out because the peer
can never start.
+ ofs << "while [ ! -f \"" << peer_marker << "\" ]; do sleep 0.01;
done\n";
Review Comment:
**[P2] Synchronize at the child-start boundary**
This file barrier still runs inside the production 500 ms
`PROCESS_START_TIMEOUT`. `start_promise` releases only the two async callers;
either caller, its detached init worker, or its shell child can be descheduled
long enough for the peer child to time out before this marker appears. That
makes one result fail even when the two version pools never share a lock, which
is indistinguishable from the regression this test targets. Please use a test
hook/latch at the actual fork/readiness boundary (before either readiness clock
starts), or inject a process factory/barrier that proves concurrent fork entry
without depending on relative OS scheduling.
##########
be/test/udf/python/python_env_test.cpp:
##########
@@ -138,50 +108,23 @@ TEST_F(PythonEnvTest,
PythonVersionIsValidWithExistingPaths) {
fs::permissions(exec_path, fs::perms::owner_all);
PythonVersion pv("3.9.16", base_path, exec_path);
- // is_valid() also checks that extract_python_version works, which
requires a real python
- // So this will fail on fake executable, but we verify paths exist
- EXPECT_TRUE(fs::exists(base_path));
- EXPECT_TRUE(fs::exists(exec_path));
-}
-
-TEST_F(PythonEnvTest, PythonVersionToString) {
- PythonVersion pv("3.9.16", "/opt/python", "/opt/python/bin/python3");
- std::string str = pv.to_string();
- EXPECT_TRUE(str.find("3.9.16") != std::string::npos);
- EXPECT_TRUE(str.find("/opt/python") != std::string::npos);
- EXPECT_TRUE(str.find("/opt/python/bin/python3") != std::string::npos);
+ EXPECT_TRUE(pv.is_valid());
}
-TEST_F(PythonEnvTest, PythonVersionHash) {
+TEST_F(PythonEnvTest, PythonVersionWorksAsProcessPoolMapKey) {
PythonVersion pv1("3.9.16", "/opt/python", "/opt/python/bin/python3");
PythonVersion pv2("3.9.16", "/opt/python", "/opt/python/bin/python3");
- PythonVersion pv3("3.10.0", "/opt/python", "/opt/python/bin/python3");
+ PythonVersion pv3("3.9.16", "/different/path",
"/different/path/bin/python3");
Review Comment:
**[P2] Keep the runtime-version dimension in this map-key test**
The replacement now checks only equal triples and the same `full_version` at
different paths. It deletes the sole case where `3.9` and `3.10` at identical
base/executable paths must remain unequal, even though `full_version` is part
of `PythonVersion::operator==` and process-pool identity. Add another key that
differs only in `full_version`, assert direct inequality, and verify a distinct
map entry. The direct assertion matters because the current hash already uses
`full_version`, so map behavior alone can mask an equality regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
