imbajin commented on code in PR #2863:
URL:
https://github.com/apache/incubator-hugegraph/pull/2863#discussion_r2313288456
##########
hugegraph-server/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStore.java:
##########
@@ -813,12 +776,56 @@ private void closeSessions() {
}
}
- private Collection<RocksDBSessions> sessions() {
+ private final Collection<RocksDBSessions> sessions() {
return this.dbs.values();
}
- private void parseTableDiskMapping(Map<String, String> disks, String
dataPath) {
+ private final List<RocksDBSessions.Session> session() {
+ this.checkDbOpened();
+
+ // Collect session of standard disk
+ RocksDBSessions.Session session = this.sessions.session();
+ if (!session.opened()) {
+ this.checkOpened();
+ }
+
+ if (this.tableDiskMapping.isEmpty()) {
+ return ImmutableList.of(session);
+ }
Review Comment:
Performance optimization looks good, but consider session state consistency.
The conditional checkOpened() call based on session.opened() could introduce
race conditions between the check and actual session usage.
##########
hugegraph-server/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStore.java:
##########
@@ -813,12 +776,56 @@ private void closeSessions() {
}
}
- private Collection<RocksDBSessions> sessions() {
+ private final Collection<RocksDBSessions> sessions() {
return this.dbs.values();
}
- private void parseTableDiskMapping(Map<String, String> disks, String
dataPath) {
+ private final List<RocksDBSessions.Session> session() {
+ this.checkDbOpened();
+
+ // Collect session of standard disk
+ RocksDBSessions.Session session = this.sessions.session();
+ if (!session.opened()) {
+ this.checkOpened();
+ }
+
+ if (this.tableDiskMapping.isEmpty()) {
+ return ImmutableList.of(session);
+ }
+ // Collect session of each table with optimized disk
+ List<RocksDBSessions.Session> list = new
ArrayList<>(this.tableDiskMapping.size() + 1);
+ list.add(session);
+ for (String disk : this.tableDiskMapping.values()) {
+ RocksDBSessions.Session optimizedSession = this.db(disk).session();
+ assert optimizedSession.opened();
Review Comment:
Inconsistent session state handling: optimized sessions use assert
optimizedSession.opened() while standard sessions get conditional checks.
Consider applying the same defensive pattern to both session types for
consistency.
##########
hugegraph-server/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStore.java:
##########
@@ -813,12 +776,56 @@ private void closeSessions() {
}
}
- private Collection<RocksDBSessions> sessions() {
+ private final Collection<RocksDBSessions> sessions() {
return this.dbs.values();
}
- private void parseTableDiskMapping(Map<String, String> disks, String
dataPath) {
+ private final List<RocksDBSessions.Session> session() {
Review Comment:
Consider making session() method final like sessions() for consistency and
to prevent accidental overrides that could break the optimized session
management logic.
##########
hugegraph-server/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStore.java:
##########
@@ -425,7 +425,6 @@ public void mutate(BackendMutation mutation) {
Lock readLock = this.storeLock.readLock();
Review Comment:
Good performance optimization removing redundant checkOpened() calls. Ensure
comprehensive testing covers store state consistency under concurrent access
and that the performance gains are measurable without introducing subtle bugs.
--
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]
