-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add replication factor column to sys table #14403
Add replication factor column to sys table #14403
Conversation
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.
Thanks for the change! I've a few questions related to API backwards compatibility and naming consistency.
sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java
Outdated
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java
Outdated
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java
Outdated
Show resolved
Hide resolved
processing/src/main/java/org/apache/druid/timeline/SegmentPlus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
Outdated
Show resolved
Hide resolved
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.
Overall approach looks good to me. One question related to setReplicationFactor
and a few minor nits, thanks!
sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java
Outdated
Show resolved
Hide resolved
processing/src/main/java/org/apache/druid/timeline/SegmentStatusInCluster.java
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java
Outdated
Show resolved
Hide resolved
processing/src/test/java/org/apache/druid/timeline/SegmentStatusInClusterTest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
Show resolved
Hide resolved
processing/src/main/java/org/apache/druid/timeline/SegmentStatusInCluster.java
Outdated
Show resolved
Hide resolved
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.
Left some final feedback comments.
@adarshsanjeev , could you please also share a web-console screenshot of the query and the results table from your local/cluster testing?
processing/src/main/java/org/apache/druid/timeline/SegmentStatusInCluster.java
Outdated
Show resolved
Hide resolved
processing/src/main/java/org/apache/druid/timeline/SegmentStatusInCluster.java
Outdated
Show resolved
Hide resolved
processing/src/test/java/org/apache/druid/timeline/SegmentStatusInClusterTest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
Outdated
Show resolved
Hide resolved
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.
Left minor comments. Overall LGTM. Will approve once those are addressed.
sql/src/main/java/org/apache/druid/sql/calcite/schema/MetadataSegmentView.java
Outdated
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/schema/MetadataSegmentView.java
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/schema/MetadataSegmentView.java
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
Show resolved
Hide resolved
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.
Looks good to me.
@adarshsanjeev , for the coverage issues, you can try adding a simple test for MetadataResource
similar to DataSourceResourceTest
or tests for other resource classes.
server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
Fixed
Show fixed
Hide fixed
server/src/test/java/org/apache/druid/server/http/MetadataResourceTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/apache/druid/server/http/MetadataResourceTest.java
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/schema/MetadataSegmentView.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/apache/druid/server/http/MetadataResourceTest.java
Show resolved
Hide resolved
Left a few minor comments, but none of them are blockers for this PR. |
This PR catches the console up to all the backend changes for Druid 27 Specifically: Add page information to SqlStatementResource API #14512 Allow empty tiered replicants map for load rules #14432 Adding Interactive API's for MSQ engine #14416 Add replication factor column to sys table #14403 Account for data format and compression in MSQ auto taskAssignment #14307 Errors take 3 #14004
This PR catches the console up to all the backend changes for Druid 27 Specifically: Add page information to SqlStatementResource API apache#14512 Allow empty tiered replicants map for load rules apache#14432 Adding Interactive API's for MSQ engine apache#14416 Add replication factor column to sys table apache#14403 Account for data format and compression in MSQ auto taskAssignment apache#14307 Errors take 3 apache#14004
This PR catches the console up to all the backend changes for Druid 27 Specifically: Add page information to SqlStatementResource API #14512 Allow empty tiered replicants map for load rules #14432 Adding Interactive API's for MSQ engine #14416 Add replication factor column to sys table #14403 Account for data format and compression in MSQ auto taskAssignment #14307 Errors take 3 #14004 Co-authored-by: Vadim Ogievetsky <vadim@ogievetsky.com>
This PR catches the console up to all the backend changes for Druid 27 Specifically: Add page information to SqlStatementResource API apache#14512 Allow empty tiered replicants map for load rules apache#14432 Adding Interactive API's for MSQ engine apache#14416 Add replication factor column to sys table apache#14403 Account for data format and compression in MSQ auto taskAssignment apache#14307 Errors take 3 apache#14004
Currently, Druid allows segments to be not loaded on any historical. This status is not tracked in the
sys.segments
table on the broker. This makes it difficult to tell from the segments table why a segment is not available, and if that is expected. This also makes it hard to decide what segments to wait for to load after an ingestion is finished, as those segments may never get loaded due to the load rules.This PR adds a new column
replication factor
to the sys virtual table and populates it with information from the coordinator. If this column is 0, the segment is not assigned to any historical and will never be loaded.This column can be access from the sys table by normal sql queries:
SELECT "segment_id", "replication_factor" FROM sys."segments"
Response:
Release Notes
replication factor
to the sys table. This returns the total number of replicants of the segment across all tiers. The column is set to -1 if the information is not available currently.