Optimize load logic#18153
Conversation
Caideyipi
left a comment
Caideyipi
left a comment
There was a problem hiding this comment.
I found two issues that should be addressed before merging:
-
The new ITs rely on observing a transient file in the active-load pending directory, but the active-load worker can scan and consume the file before the assertion runs. This is already failing in CI:
IoTDBLoadTsFileActiveRetryIT.testAsyncLoadShouldStoreUserInfoInActiveDirfails at line 166, andIoTDBLoadTsFileAuthIT.testAsyncLoadShouldMoveFileToFailDirWhenUserDoesNotExistfails at line 207. Please avoid asserting on this racing pending-file state, or make the loader/scanner behavior deterministic for the test. -
The PR adds new user-facing raw English strings in production code:
ActiveLoadTsFileLoader:The user in the active load path does not existActiveLoadPathHelper:User name must not be empty
Per the repo i18n rule, these should be moved to the corresponding
StorageEngineMessagesconstants in bothenandzh, then referenced from code.
The core permission-checking approach looks reasonable to me, but the CI-breaking flaky tests and the i18n literals should be fixed first.
luoluoyuyu
force-pushed
the
fix-load-write-data-auth
branch
2 times, most recently
from
2c0d324 to
8ae23f2
Compare
| return USER_VALUE_MASK_PREFIX | ||
| + Base64.getUrlEncoder() | ||
| .withoutPadding() | ||
| .encodeToString(value.getBytes(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Better to use Base32 since some file systems are case-insensitive,
| break; | ||
| case USER_KEY: | ||
| if (value == null || value.isEmpty()) { | ||
| throw new SemanticException("User name must not be empty"); |
3 participants
Description
As the title said
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR