You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "javeme (via GitHub)" <gi...@apache.org> on 2025/08/30 18:40:20 UTC
[PR] perf(rocksdb): RocksDBStore remove redundant checkOpened() call [incubator-hugegraph]
javeme opened a new pull request, #2863:
URL: https://github.com/apache/incubator-hugegraph/pull/2863
Change-Id: I4c4ae6368fecb994708ae1da89c256beefda4511
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(rocksdb): RocksDBStore remove redundant checkOpened() call [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(rocksdb): RocksDBStore remove redundant checkOpened() call [incubator-hugegraph]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2863:
URL: https://github.com/apache/incubator-hugegraph/pull/2863#discussion_r2328691178
##########
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:
It is believed here that optimizedSession must be the opened state, otherwise it is a bug in logic. We consider this optimizedSession state to be consistent with the main session state.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(rocksdb): RocksDBStore remove redundant checkOpened() call [incubator-hugegraph]
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies merged PR #2863:
URL: https://github.com/apache/incubator-hugegraph/pull/2863
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(rocksdb): RocksDBStore remove redundant checkOpened() call [incubator-hugegraph]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2863:
URL: https://github.com/apache/incubator-hugegraph/pull/2863#issuecomment-3239455366
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2863?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `34.78261%` with `15 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 33.80%. Comparing base ([`0946d5d`](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/0946d5dd0cad8c27cb365b0c2aaaf217cdca3402?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`28b969f`](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/28b969fb9592eddd1bfcdb5e609d94458d288f05?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2863?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../hugegraph/backend/store/rocksdb/RocksDBStore.java](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2863?src=pr&el=tree&filepath=hugegraph-server%2Fhugegraph-rocksdb%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Fbackend%2Fstore%2Frocksdb%2FRocksDBStore.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLXNlcnZlci9odWdlZ3JhcGgtcm9ja3NkYi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2JhY2tlbmQvc3RvcmUvcm9ja3NkYi9Sb2Nrc0RCU3RvcmUuamF2YQ==) | 34.78% | [11 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2863?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (0946d5d) and HEAD (28b969f). Click for more details.
>
> <details><summary>HEAD has 2 uploads less than BASE</summary>
>
>| Flag | BASE (0946d5d) | HEAD (28b969f) |
>|------|------|------|
>||3|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #2863 +/- ##
============================================
- Coverage 40.91% 33.80% -7.11%
+ Complexity 333 264 -69
============================================
Files 747 747
Lines 60168 60170 +2
Branches 7683 7686 +3
============================================
- Hits 24615 20340 -4275
- Misses 32975 37536 +4561
+ Partials 2578 2294 -284
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2863?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org