ibessonov commented on code in PR #1929:
URL: https://github.com/apache/ignite-3/pull/1929#discussion_r1173870250
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationFlattener.java:
##########
@@ -136,6 +136,8 @@ public Void doVisitInnerNode(String key, InnerNode newNode)
{
return null;
}
+ // in case inner node is null in both trees,
Review Comment:
I think I mentioned guidelines somewhere, but I'll repeat. First letter is
capitalizes, sentence ends with a dot. This is how we should write comments.
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java:
##########
@@ -147,49 +139,40 @@ private Config readHoconFromFile() {
@Override
public CompletableFuture<Map<String, ? extends Serializable>>
readAllLatest(String prefix) {
- return async(() -> {
- lock.readLock().lock();
- try {
- return latest.entrySet()
- .stream()
- .filter(entry -> entry.getKey().startsWith(prefix))
- .collect(toMap(Entry::getKey, Entry::getValue));
- } finally {
- lock.readLock().unlock();
- }
- });
+ lock.readLock().lock();
+ try {
+ return CompletableFuture.completedFuture(
+ latest.entrySet()
+ .stream()
+ .filter(entry -> entry.getKey().startsWith(prefix))
+ .collect(toMap(Entry::getKey, Entry::getValue))
+ );
+ } finally {
+ lock.readLock().unlock();
+ }
}
@Override
public CompletableFuture<Serializable> readLatest(String key) {
- return async(() -> {
- lock.readLock().lock();
- try {
- return latest.get(key);
- } finally {
- lock.readLock().unlock();
- }
- });
+ return CompletableFuture.completedFuture(latest.get(key));
Review Comment:
Why have you removed the read-lock? Technically, you lost the property of
the data being "latest". By that I mean that two consecutive calls can return
newer value first, and older value next, if you ask them not in the order that
they are being inserted in "write" at the same time. Read lock would save you
from such problems.
Please be careful in such places and don't forget about broader invariants.
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorageTest.java:
##########
@@ -413,10 +411,12 @@ void deleteFileBeforeReadAll() throws Exception {
/** Read configuration when inner node configured with partial content
(some fields are empty). */
@Test
void innerNodeWithPartialContent() throws Exception {
+ // Given
String content = "top: { inner.boolVal: true }";
Files.write(getConfigFile(), content.getBytes(StandardCharsets.UTF_8));
- storage.readDataOnRecovery().get();
+ // Expect
+ assertThat(storage.readDataOnRecovery().get().values(),
allOf(aMapWithSize(1)));
Review Comment:
Any reason to have "allOf" for a single matcher?
--
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]
