-
Notifications
You must be signed in to change notification settings - Fork 448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(interactive): Support Statistics in Groot Storage #3856
Conversation
Committed-by: bingqing.lbq from Dev container
Committed-by: bingqing.lbq from Dev container
Committed-by: bingqing.lbq from Dev container
Committed-by: bingqing.lbq from Dev container
Committed-by: bingqing.lbq from Dev container
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please separate getting statistics from getting schema.
self.version | ||
} | ||
|
||
pub fn get_vertex_count(&self) -> i64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these counts all return i64
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is refined to be u64
.
pub fn to_proto(&self) -> GraphResult<StatisticsPb> { | ||
let mut pb = StatisticsPb::new(); | ||
pb.set_snapshotId(self.version); | ||
pb.set_numVertices(self.vertex_count as u64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and these count u64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
throw new RuntimeException(e); | ||
SnapshotId si = new SnapshotId(true, snapshotId); | ||
IrGraphSchema irSchema = new IrGraphSchema(schema, true); | ||
GraphStatistics statistics = this.schemaFetcher.getStatistics(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not okay to place getting statistics with schema for now. For larger graph, getting statistics is a very time-costly operation, and it is operated at the frequency as schema? I don't think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have separated the two interfaces for getting schema (+procedures) and statistics.
Committed-by: bingqing.lbq from Dev container
Executors.newSingleThreadScheduledExecutor( | ||
ThreadFactoryUtils.daemonThreadFactoryWithLogExceptionHandler( | ||
"fetch-statistics", logger)); | ||
this.fetchStatisticsScheduler.scheduleWithFixedDelay( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fetching logic is a bit messy. The sendstatisticstofrontend can always be triggered after fetchstatistics from store, rather than two individual threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by triggering send after fetch, and send scheduler is removed.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3856 +/- ##
===========================================
- Coverage 46.22% 26.02% -20.21%
===========================================
Files 174 192 +18
Lines 16172 17357 +1185
===========================================
- Hits 7476 4517 -2959
- Misses 8696 12840 +4144 see 83 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What do these changes do?
As titled.
Related issue number
#3843