You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/08/05 01:30:51 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request #7255: Unify segment ZK metadata
Jackie-Jiang opened a new pull request #7255:
URL: https://github.com/apache/pinot/pull/7255
## Description
Currently there are 3 different classes for segment ZK metadata: `OfflineSegmentZKMetadata`, `RealtimeSegmentZKMetadata`, `LLCRealtimeSegmentZKMetadata`, and 3 different sets of APIs to read them from the ZK.
The problem with this approach:
- Need to know the type of segment before reading the ZK metadata, which is hard to use and makes common code non-sharable
- Impossible to mix different types of ZK metadata. E.g. to support segment push to LLC table
This PR unifies these 3 classes to `SegmentZKMetadata` class which is backed by the `ZNRecord` to:
- Solve the problems above
- Read fields from `ZNRecord` on demand to avoid pre-read all fields (in most cases only one or two fields are required)
- Directly write the fields back to `ZNRecord` to avoid the overhead of creating a new `ZNRecord` when persisting back the segment ZK metadata
The following keys are deprecated, backward-compatibility is maintained:
- `segment.realtime.download.url` and `segment.offline.download.url`: replaced by `segment.download.url`
- `segment.offline.push.time`: replaced by `segment.push.time`
- `segment.offline.refresh.time`: replaced by `segment.refresh.time`
- `segment.name`: use ZNRecord id instead
- `segment.table.name`: decouple segment from table
- `segment.type`: no need to differentiate
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-893851344
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7255](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (916b490) into [master](https://codecov.io/gh/apache/pinot/commit/066c79399922fecc428ff05b2b983ef7367512d3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (066c793) will **increase** coverage by `0.03%`.
> The diff coverage is `77.56%`.
> :exclamation: Current head 916b490 differs from pull request most recent head 14fd351. Consider uploading reports for the commit 14fd351 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7255 +/- ##
============================================
+ Coverage 73.53% 73.57% +0.03%
Complexity 92 92
============================================
Files 1508 1505 -3
Lines 73999 73743 -256
Branches 10691 10665 -26
============================================
- Hits 54413 54253 -160
+ Misses 16020 15924 -96
Partials 3566 3566
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.88% <62.97%> (+0.07%)` | :arrow_up: |
| unittests | `65.31% <59.72%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...realtime/segment/DefaultFlushThresholdUpdater.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL3NlZ21lbnQvRGVmYXVsdEZsdXNoVGhyZXNob2xkVXBkYXRlci5qYXZh) | `100.00% <ø> (ø)` | |
| [...segment/SegmentSizeBasedFlushThresholdUpdater.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL3NlZ21lbnQvU2VnbWVudFNpemVCYXNlZEZsdXNoVGhyZXNob2xkVXBkYXRlci5qYXZh) | `70.58% <ø> (ø)` | |
| [...pinot/controller/util/TableRetentionValidator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL1RhYmxlUmV0ZW50aW9uVmFsaWRhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...nd\_push/SegmentGenerationAndPushTaskGenerator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3Mvc2VnbWVudF9nZW5lcmF0aW9uX2FuZF9wdXNoL1NlZ21lbnRHZW5lcmF0aW9uQW5kUHVzaFRhc2tHZW5lcmF0b3IuamF2YQ==) | `8.49% <0.00%> (-0.06%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `35.00% <ø> (ø)` | |
| [...apache/pinot/controller/api/upload/ZKOperator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvdXBsb2FkL1pLT3BlcmF0b3IuamF2YQ==) | `75.55% <50.00%> (+0.27%)` | :arrow_up: |
| [...not/common/metadata/segment/SegmentZKMetadata.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0YWRhdGEvc2VnbWVudC9TZWdtZW50WktNZXRhZGF0YS5qYXZh) | `70.83% <69.16%> (-8.50%)` | :arrow_down: |
| [.../helix/core/realtime/SegmentCompletionManager.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1NlZ21lbnRDb21wbGV0aW9uTWFuYWdlci5qYXZh) | `72.24% <69.23%> (+0.11%)` | :arrow_up: |
| [...ler/api/resources/PinotSegmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90U2VnbWVudFJlc3RsZXRSZXNvdXJjZS5qYXZh) | `20.32% <80.00%> (+0.84%)` | :arrow_up: |
| [...r/validation/RealtimeSegmentValidationManager.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL1JlYWx0aW1lU2VnbWVudFZhbGlkYXRpb25NYW5hZ2VyLmphdmE=) | `83.60% <85.71%> (-0.27%)` | :arrow_down: |
| ... and [52 more](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [066c793...14fd351](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r686337781
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotRealtimeSegmentManager.java
##########
@@ -201,15 +201,13 @@ private synchronized void assignRealtimeSegmentsToServerInstancesIfNecessary() {
for (String partition : state.getPartitionSet()) {
// Helix partition is the segment name
if (SegmentName.isHighLevelConsumerSegmentName(partition)) {
- HLCSegmentName segName = new HLCSegmentName(partition);
- RealtimeSegmentZKMetadata realtimeSegmentZKMetadata = ZKMetadataProvider
- .getRealtimeSegmentZKMetadata(_pinotHelixResourceManager.getPropertyStore(), segName.getTableName(),
- partition);
- if (realtimeSegmentZKMetadata == null) {
+ SegmentZKMetadata segmentZKMetadata = ZKMetadataProvider
Review comment:
partition is the same as segment name (mentioned in the comment)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r686335787
##########
File path: pinot-common/src/test/java/org/apache/pinot/common/tier/TierSegmentSelectorTest.java
##########
@@ -46,8 +45,8 @@ public void testTimeBasedSegmentSelector() {
// offline segment
String segmentName = "segment_0";
String tableNameWithType = "myTable_OFFLINE";
- OfflineSegmentZKMetadata offlineSegmentZKMetadata = new OfflineSegmentZKMetadata();
- offlineSegmentZKMetadata.setSegmentName(segmentName);
+ SegmentZKMetadata offlineSegmentZKMetadata = new SegmentZKMetadata(segmentName);
+ offlineSegmentZKMetadata.setSegmentType(CommonConstants.Segment.SegmentType.OFFLINE);
Review comment:
It is called within the constructor of `OfflineSegmentZKMetadata` and `RealtimeSegmentZKMetadata` which is being cleaned up in this PR. It is already deprecated and will be removed in the next release, so I didn't put it into the constructor so that it can be easily cleaned up
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
klsince commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r685466546
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java
##########
@@ -136,14 +135,14 @@ static long computeRealtimeTotalDocumentInSegments(List<RealtimeSegmentZKMetadat
groupId = segmentGroupIdName;
}
// Discard all segments with different groupids as they are replicas
- if (groupId.equals(segmentGroupIdName) && realtimeSegmentZKMetadata.getTotalDocs() >= 0) {
- numTotalDocs += realtimeSegmentZKMetadata.getTotalDocs();
+ if (groupId.equals(segmentGroupIdName) && segmentZKMetadata.getTotalDocs() >= 0) {
+ numTotalDocs += segmentZKMetadata.getTotalDocs();
}
}
} else {
Review comment:
is this else and the if-check below redundant?
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java
##########
@@ -90,7 +91,7 @@
static final long START_TIME_MS = CURRENT_TIME_MS - TimeUnit.HOURS.toMillis(RANDOM.nextInt(24) + 24);
static final long END_TIME_MS = START_TIME_MS + TimeUnit.HOURS.toMillis(RANDOM.nextInt(24) + 1);
static final Interval INTERVAL = new Interval(START_TIME_MS, END_TIME_MS);
- static final String CRC = Long.toString(RANDOM.nextLong());
+ static final String CRC = Long.toString(RANDOM.nextLong() & 0xFFFFFFFFL);
Review comment:
curious why `& 0xFFFFFFFFL` here?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java
##########
@@ -36,209 +34,291 @@
import org.slf4j.LoggerFactory;
-public abstract class SegmentZKMetadata implements ZKMetadata {
+public class SegmentZKMetadata implements ZKMetadata {
Review comment:
looks like changes in this file can be in its own PR to merge firstly? as the interfaces are not changed much.
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java
##########
@@ -93,7 +92,8 @@
private static final long TIMEOUT_IN_MS = 10_000L;
@BeforeClass
- public void setUp() throws Exception {
+ public void setUp()
Review comment:
looks like the changes are mostly on formats. how about open a separate RP to land those first?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java
##########
@@ -377,10 +377,9 @@ public void run() {
try {
_segmentLogger.info("Marking current segment as completed in Helix");
- RealtimeSegmentZKMetadata metadataToOverwrite = new RealtimeSegmentZKMetadata();
+ SegmentZKMetadata metadataToOverwrite = new SegmentZKMetadata(segmentZKMetadata.getSegmentName());
metadataToOverwrite.setTableName(_tableNameWithType);
- metadataToOverwrite.setSegmentName(realtimeSegmentZKMetadata.getSegmentName());
- metadataToOverwrite.setSegmentType(SegmentType.OFFLINE);
Review comment:
hmm.. is this a typo in the old code, or intentionally?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/util/TableRetentionValidator.java
##########
@@ -128,54 +128,55 @@ public void run()
try {
timeUnit = TimeUnit.valueOf(timeUnitString.toUpperCase());
} catch (Exception e) {
- LOGGER.error("Table: {}, invalid time unit: {}", tableName, timeUnitString);
+ LOGGER.error("Table: {}, invalid time unit: {}", tableNameWithType, timeUnitString);
continue;
}
// Get time duration in days
String timeValueString = retentionConfig.getRetentionTimeValue();
long durationInDays;
try {
- durationInDays = timeUnit.toDays(Long.valueOf(timeValueString));
+ durationInDays = timeUnit.toDays(Long.parseLong(timeValueString));
} catch (Exception e) {
- LOGGER.error("Table: {}, invalid time value: {}", tableName, timeValueString);
+ LOGGER.error("Table: {}, invalid time value: {}", tableNameWithType, timeValueString);
continue;
}
if (durationInDays <= 0) {
- LOGGER.error("Table: {}, invalid retention duration in days: {}", tableName, durationInDays);
+ LOGGER.error("Table: {}, invalid retention duration in days: {}", tableNameWithType, durationInDays);
continue;
}
if (durationInDays > _durationInDaysThreshold) {
- LOGGER.warn("Table: {}, retention duration in days is too large: {}", tableName, durationInDays);
+ LOGGER.warn("Table: {}, retention duration in days is too large: {}", tableNameWithType, durationInDays);
}
// Skip segments metadata check for realtime tables
- if (tableName.endsWith("REALTIME")) {
+ if (tableNameWithType.endsWith("REALTIME")) {
Review comment:
iirc, there is util method to check table type? then may avoid hard coding "REALTIME" here.
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/SegmentCompletionTest.java
##########
@@ -64,7 +64,8 @@
private final LongMsgOffset s3Offset = new LongMsgOffset(30L);
@BeforeMethod
- public void testCaseSetup() throws Exception {
+ public void testCaseSetup()
Review comment:
changes in this file are also mainly on formats. The changes are so many that github simply folds the file up. would suggest to open a quick RP to land those format changes firstly.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r686332582
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
##########
@@ -309,89 +272,27 @@ public static Schema getTableSchema(@Nonnull ZkHelixPropertyStore<ZNRecord> prop
* NOTE: this method is very expensive, use {@link #getSegments(ZkHelixPropertyStore, String)} instead if only segment
* segment names are needed.
*/
- public static List<OfflineSegmentZKMetadata> getOfflineSegmentZKMetadataListForTable(
- ZkHelixPropertyStore<ZNRecord> propertyStore, String tableName) {
- String offlineTableName = TableNameBuilder.OFFLINE.tableNameWithType(tableName);
- String parentPath = constructPropertyStorePathForResource(offlineTableName);
- List<ZNRecord> znRecords = propertyStore.getChildren(parentPath, null, AccessOption.PERSISTENT,
- CommonConstants.Helix.ZkClient.RETRY_COUNT, CommonConstants.Helix.ZkClient.RETRY_INTERVAL_MS);
+ public static List<SegmentZKMetadata> getSegmentsZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
+ String tableNameWithType) {
+ String parentPath = constructPropertyStorePathForResource(tableNameWithType);
+ List<ZNRecord> znRecords = propertyStore
+ .getChildren(parentPath, null, AccessOption.PERSISTENT, CommonConstants.Helix.ZkClient.RETRY_COUNT,
+ CommonConstants.Helix.ZkClient.RETRY_INTERVAL_MS);
if (znRecords != null) {
int numZNRecords = znRecords.size();
- List<OfflineSegmentZKMetadata> offlineSegmentZKMetadataList = new ArrayList<>(numZNRecords);
+ List<SegmentZKMetadata> segmentsZKMetadata = new ArrayList<>(numZNRecords);
for (ZNRecord znRecord : znRecords) {
// NOTE: it is possible that znRecord is null if the record gets removed while calling this method
if (znRecord != null) {
- offlineSegmentZKMetadataList.add(new OfflineSegmentZKMetadata(znRecord));
+ segmentsZKMetadata.add(new SegmentZKMetadata(znRecord));
}
}
- int numNullZNRecords = numZNRecords - offlineSegmentZKMetadataList.size();
+ int numNullZNRecords = numZNRecords - segmentsZKMetadata.size();
if (numNullZNRecords > 0) {
- LOGGER.warn("Failed to read {}/{} offline segment ZK metadata under path: {}", numZNRecords - numNullZNRecords,
+ LOGGER.warn("Failed to read {}/{} segment ZK metadata under path: {}", numZNRecords - numNullZNRecords,
Review comment:
Good catch
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r685382782
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
##########
@@ -144,34 +141,25 @@ public static void removeResourceConfigFromPropertyStore(ZkHelixPropertyStore<ZN
}
}
- public static boolean setOfflineSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String offlineTableName, OfflineSegmentZKMetadata offlineSegmentZKMetadata, int expectedVersion) {
+ public static boolean setSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore, String tableNameWithType,
+ SegmentZKMetadata segmentZKMetadata, int expectedVersion) {
// NOTE: Helix will throw ZkBadVersionException if version does not match
try {
return propertyStore
- .set(constructPropertyStorePathForSegment(offlineTableName, offlineSegmentZKMetadata.getSegmentName()),
- offlineSegmentZKMetadata.toZNRecord(), expectedVersion, AccessOption.PERSISTENT);
+ .set(constructPropertyStorePathForSegment(tableNameWithType, segmentZKMetadata.getSegmentName()),
+ segmentZKMetadata.toZNRecord(), expectedVersion, AccessOption.PERSISTENT);
} catch (ZkBadVersionException e) {
return false;
}
}
- public static boolean setOfflineSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String offlineTableName, OfflineSegmentZKMetadata offlineSegmentZKMetadata) {
- return propertyStore
- .set(constructPropertyStorePathForSegment(offlineTableName, offlineSegmentZKMetadata.getSegmentName()),
- offlineSegmentZKMetadata.toZNRecord(), AccessOption.PERSISTENT);
- }
-
- public static boolean setRealtimeSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String realtimeTableName, RealtimeSegmentZKMetadata realtimeSegmentZKMetadata) {
- return propertyStore
- .set(constructPropertyStorePathForSegment(realtimeTableName, realtimeSegmentZKMetadata.getSegmentName()),
- realtimeSegmentZKMetadata.toZNRecord(), AccessOption.PERSISTENT);
+ public static boolean setSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore, String tableNameWithType,
+ SegmentZKMetadata segmentZKMetadata) {
+ return setSegmentZKMetadata(propertyStore, tableNameWithType, segmentZKMetadata, -1);
}
@Nullable
- public static ZNRecord getZnRecord(@Nonnull ZkHelixPropertyStore<ZNRecord> propertyStore, @Nonnull String path) {
+ public static ZNRecord getZnRecord(ZkHelixPropertyStore<ZNRecord> propertyStore, String path) {
Review comment:
`Nonnull` and `Nullable` looks quite similar, so we usually only keep `Nullable` and assume the variables are `Nonnull` by default (google convention)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-893851344
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7255](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d673c12) into [master](https://codecov.io/gh/apache/pinot/commit/6fb51d2889e5b269e52f483f3cac89f5562a3a62?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6fb51d2) will **decrease** coverage by `33.38%`.
> The diff coverage is `70.68%`.
> :exclamation: Current head d673c12 differs from pull request most recent head cb5dfb2. Consider uploading reports for the commit cb5dfb2 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7255 +/- ##
=============================================
- Coverage 71.33% 37.95% -33.39%
+ Complexity 3363 92 -3271
=============================================
Files 1505 1502 -3
Lines 74224 73984 -240
Branches 10733 10709 -24
=============================================
- Hits 52950 28077 -24873
- Misses 17678 43680 +26002
+ Partials 3596 2227 -1369
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `29.36% <52.71%> (+0.12%)` | :arrow_up: |
| unittests1 | `?` | |
| unittests2 | `14.65% <33.49%> (+0.03%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...inot/common/tier/TimeBasedTierSegmentSelector.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaW1lQmFzZWRUaWVyU2VnbWVudFNlbGVjdG9yLmphdmE=) | `0.00% <0.00%> (-94.45%)` | :arrow_down: |
| [...ler/api/resources/PinotSegmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90U2VnbWVudFJlc3RsZXRSZXNvdXJjZS5qYXZh) | `16.14% <0.00%> (-7.33%)` | :arrow_down: |
| [...realtime/segment/DefaultFlushThresholdUpdater.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL3NlZ21lbnQvRGVmYXVsdEZsdXNoVGhyZXNob2xkVXBkYXRlci5qYXZh) | `100.00% <ø> (ø)` | |
| [...segment/SegmentSizeBasedFlushThresholdUpdater.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL3NlZ21lbnQvU2VnbWVudFNpemVCYXNlZEZsdXNoVGhyZXNob2xkVXBkYXRlci5qYXZh) | `70.58% <ø> (ø)` | |
| [...pinot/controller/util/TableRetentionValidator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL1RhYmxlUmV0ZW50aW9uVmFsaWRhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...he/pinot/plugin/minion/tasks/BaseTaskExecutor.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvQmFzZVRhc2tFeGVjdXRvci5qYXZh) | `47.05% <0.00%> (-29.42%)` | :arrow_down: |
| [...t\_to\_raw\_index/ConvertToRawIndexTaskGenerator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvY29udmVydF90b19yYXdfaW5kZXgvQ29udmVydFRvUmF3SW5kZXhUYXNrR2VuZXJhdG9yLmphdmE=) | `10.41% <0.00%> (-77.34%)` | :arrow_down: |
| [...nd\_push/SegmentGenerationAndPushTaskGenerator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3Mvc2VnbWVudF9nZW5lcmF0aW9uX2FuZF9wdXNoL1NlZ21lbnRHZW5lcmF0aW9uQW5kUHVzaFRhc2tHZW5lcmF0b3IuamF2YQ==) | `8.55% <0.00%> (ø)` | |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `0.00% <0.00%> (-58.35%)` | :arrow_down: |
| [...ent/local/realtime/impl/RealtimeSegmentConfig.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL1JlYWx0aW1lU2VnbWVudENvbmZpZy5qYXZh) | `0.00% <0.00%> (-91.87%)` | :arrow_down: |
| ... and [887 more](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d40e2d3...cb5dfb2](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r687264829
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/tier/TimeBasedTierSegmentSelector.java
##########
@@ -55,10 +55,10 @@ public boolean selectSegment(String tableNameWithType, String segmentName) {
SegmentZKMetadata segmentZKMetadata;
if (TableNameBuilder.isOfflineTableResource(tableNameWithType)) {
Review comment:
we don't need this if check anymore
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r686340719
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java
##########
@@ -93,7 +92,8 @@
private static final long TIMEOUT_IN_MS = 10_000L;
@BeforeClass
- public void setUp() throws Exception {
+ public void setUp()
Review comment:
Sure, will make a PR to only reformat
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-893851344
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7255](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fa6a221) into [master](https://codecov.io/gh/apache/pinot/commit/066c79399922fecc428ff05b2b983ef7367512d3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (066c793) will **decrease** coverage by `0.01%`.
> The diff coverage is `78.36%`.
[](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7255 +/- ##
============================================
- Coverage 73.53% 73.52% -0.02%
Complexity 92 92
============================================
Files 1508 1505 -3
Lines 73999 73761 -238
Branches 10691 10668 -23
============================================
- Hits 54413 54230 -183
+ Misses 16020 15971 -49
+ Partials 3566 3560 -6
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.84% <65.17%> (+0.04%)` | :arrow_up: |
| unittests | `65.35% <61.47%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...realtime/segment/DefaultFlushThresholdUpdater.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL3NlZ21lbnQvRGVmYXVsdEZsdXNoVGhyZXNob2xkVXBkYXRlci5qYXZh) | `100.00% <ø> (ø)` | |
| [...segment/SegmentSizeBasedFlushThresholdUpdater.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL3NlZ21lbnQvU2VnbWVudFNpemVCYXNlZEZsdXNoVGhyZXNob2xkVXBkYXRlci5qYXZh) | `70.58% <ø> (ø)` | |
| [...pinot/controller/util/TableRetentionValidator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL1RhYmxlUmV0ZW50aW9uVmFsaWRhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...nd\_push/SegmentGenerationAndPushTaskGenerator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3Mvc2VnbWVudF9nZW5lcmF0aW9uX2FuZF9wdXNoL1NlZ21lbnRHZW5lcmF0aW9uQW5kUHVzaFRhc2tHZW5lcmF0b3IuamF2YQ==) | `8.49% <0.00%> (-0.06%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `35.00% <ø> (ø)` | |
| [...apache/pinot/controller/api/upload/ZKOperator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvdXBsb2FkL1pLT3BlcmF0b3IuamF2YQ==) | `75.55% <50.00%> (+0.27%)` | :arrow_up: |
| [.../helix/core/realtime/SegmentCompletionManager.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1NlZ21lbnRDb21wbGV0aW9uTWFuYWdlci5qYXZh) | `73.26% <69.23%> (+1.13%)` | :arrow_up: |
| [...not/common/metadata/segment/SegmentZKMetadata.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0YWRhdGEvc2VnbWVudC9TZWdtZW50WktNZXRhZGF0YS5qYXZh) | `74.02% <72.86%> (-5.31%)` | :arrow_down: |
| [...ler/api/resources/PinotSegmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90U2VnbWVudFJlc3RsZXRSZXNvdXJjZS5qYXZh) | `20.32% <80.00%> (+0.84%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `62.35% <82.35%> (-0.09%)` | :arrow_down: |
| ... and [52 more](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [066c793...fa6a221](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-893851344
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7255](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d673c12) into [master](https://codecov.io/gh/apache/pinot/commit/6fb51d2889e5b269e52f483f3cac89f5562a3a62?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6fb51d2) will **decrease** coverage by `56.68%`.
> The diff coverage is `33.49%`.
> :exclamation: Current head d673c12 differs from pull request most recent head cb5dfb2. Consider uploading reports for the commit cb5dfb2 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7255 +/- ##
=============================================
- Coverage 71.33% 14.65% -56.69%
+ Complexity 3363 92 -3271
=============================================
Files 1505 1456 -49
Lines 74224 72074 -2150
Branches 10733 10505 -228
=============================================
- Hits 52950 10561 -42389
- Misses 17678 60749 +43071
+ Partials 3596 764 -2832
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.65% <33.49%> (+0.03%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ache/pinot/common/metadata/ZKMetadataProvider.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0YWRhdGEvWktNZXRhZGF0YVByb3ZpZGVyLmphdmE=) | `0.00% <0.00%> (-76.64%)` | :arrow_down: |
| [...not/common/metadata/segment/SegmentZKMetadata.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0YWRhdGEvc2VnbWVudC9TZWdtZW50WktNZXRhZGF0YS5qYXZh) | `0.00% <0.00%> (-76.00%)` | :arrow_down: |
| [...inot/common/tier/TimeBasedTierSegmentSelector.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaW1lQmFzZWRUaWVyU2VnbWVudFNlbGVjdG9yLmphdmE=) | `0.00% <0.00%> (-94.45%)` | :arrow_down: |
| [...va/org/apache/pinot/common/utils/SegmentUtils.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvU2VnbWVudFV0aWxzLmphdmE=) | `0.00% <0.00%> (-68.75%)` | :arrow_down: |
| [...ler/api/resources/PinotSegmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90U2VnbWVudFJlc3RsZXRSZXNvdXJjZS5qYXZh) | `3.64% <0.00%> (-19.83%)` | :arrow_down: |
| [...troller/helix/core/minion/ClusterInfoAccessor.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9DbHVzdGVySW5mb0FjY2Vzc29yLmphdmE=) | `36.00% <0.00%> (-30.67%)` | :arrow_down: |
| [...realtime/segment/DefaultFlushThresholdUpdater.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL3NlZ21lbnQvRGVmYXVsdEZsdXNoVGhyZXNob2xkVXBkYXRlci5qYXZh) | `100.00% <ø> (ø)` | |
| [...segment/SegmentSizeBasedFlushThresholdUpdater.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL3NlZ21lbnQvU2VnbWVudFNpemVCYXNlZEZsdXNoVGhyZXNob2xkVXBkYXRlci5qYXZh) | `70.58% <ø> (ø)` | |
| [...pinot/controller/util/TableRetentionValidator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL1RhYmxlUmV0ZW50aW9uVmFsaWRhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...ller/validation/OfflineSegmentIntervalChecker.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL09mZmxpbmVTZWdtZW50SW50ZXJ2YWxDaGVja2VyLmphdmE=) | `29.11% <0.00%> (-57.14%)` | :arrow_down: |
| ... and [1208 more](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d40e2d3...cb5dfb2](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
klsince commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r685440074
##########
File path: pinot-common/src/test/java/org/apache/pinot/common/tier/TierSegmentSelectorTest.java
##########
@@ -82,8 +81,8 @@ public void testTimeBasedSegmentSelector() {
// realtime segment
segmentName = "myTable__4__1__" + now;
tableNameWithType = "myTable_REALTIME";
- LLCRealtimeSegmentZKMetadata realtimeSegmentZKMetadata = new LLCRealtimeSegmentZKMetadata();
- realtimeSegmentZKMetadata.setSegmentName(segmentName);
+ SegmentZKMetadata realtimeSegmentZKMetadata = new SegmentZKMetadata(segmentName);
+ offlineSegmentZKMetadata.setSegmentType(CommonConstants.Segment.SegmentType.REALTIME);
Review comment:
realtimeSegmentZKMetadata?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
##########
@@ -309,89 +272,27 @@ public static Schema getTableSchema(@Nonnull ZkHelixPropertyStore<ZNRecord> prop
* NOTE: this method is very expensive, use {@link #getSegments(ZkHelixPropertyStore, String)} instead if only segment
* segment names are needed.
Review comment:
typo `... only segment segment names...`
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
##########
@@ -309,89 +272,27 @@ public static Schema getTableSchema(@Nonnull ZkHelixPropertyStore<ZNRecord> prop
* NOTE: this method is very expensive, use {@link #getSegments(ZkHelixPropertyStore, String)} instead if only segment
* segment names are needed.
*/
- public static List<OfflineSegmentZKMetadata> getOfflineSegmentZKMetadataListForTable(
- ZkHelixPropertyStore<ZNRecord> propertyStore, String tableName) {
- String offlineTableName = TableNameBuilder.OFFLINE.tableNameWithType(tableName);
- String parentPath = constructPropertyStorePathForResource(offlineTableName);
- List<ZNRecord> znRecords = propertyStore.getChildren(parentPath, null, AccessOption.PERSISTENT,
- CommonConstants.Helix.ZkClient.RETRY_COUNT, CommonConstants.Helix.ZkClient.RETRY_INTERVAL_MS);
+ public static List<SegmentZKMetadata> getSegmentsZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
+ String tableNameWithType) {
+ String parentPath = constructPropertyStorePathForResource(tableNameWithType);
+ List<ZNRecord> znRecords = propertyStore
+ .getChildren(parentPath, null, AccessOption.PERSISTENT, CommonConstants.Helix.ZkClient.RETRY_COUNT,
+ CommonConstants.Helix.ZkClient.RETRY_INTERVAL_MS);
if (znRecords != null) {
int numZNRecords = znRecords.size();
- List<OfflineSegmentZKMetadata> offlineSegmentZKMetadataList = new ArrayList<>(numZNRecords);
+ List<SegmentZKMetadata> segmentsZKMetadata = new ArrayList<>(numZNRecords);
for (ZNRecord znRecord : znRecords) {
// NOTE: it is possible that znRecord is null if the record gets removed while calling this method
if (znRecord != null) {
- offlineSegmentZKMetadataList.add(new OfflineSegmentZKMetadata(znRecord));
+ segmentsZKMetadata.add(new SegmentZKMetadata(znRecord));
}
}
- int numNullZNRecords = numZNRecords - offlineSegmentZKMetadataList.size();
+ int numNullZNRecords = numZNRecords - segmentsZKMetadata.size();
if (numNullZNRecords > 0) {
- LOGGER.warn("Failed to read {}/{} offline segment ZK metadata under path: {}", numZNRecords - numNullZNRecords,
+ LOGGER.warn("Failed to read {}/{} segment ZK metadata under path: {}", numZNRecords - numNullZNRecords,
Review comment:
per the log msg, it feels like the first {} should be filled with 'numNullZnRecord'?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotRealtimeSegmentManager.java
##########
@@ -201,15 +201,13 @@ private synchronized void assignRealtimeSegmentsToServerInstancesIfNecessary() {
for (String partition : state.getPartitionSet()) {
// Helix partition is the segment name
if (SegmentName.isHighLevelConsumerSegmentName(partition)) {
- HLCSegmentName segName = new HLCSegmentName(partition);
- RealtimeSegmentZKMetadata realtimeSegmentZKMetadata = ZKMetadataProvider
- .getRealtimeSegmentZKMetadata(_pinotHelixResourceManager.getPropertyStore(), segName.getTableName(),
- partition);
- if (realtimeSegmentZKMetadata == null) {
+ SegmentZKMetadata segmentZKMetadata = ZKMetadataProvider
Review comment:
looks like segName was not used in the old code. but should it be used actually? segName vs. partition in the getSegmentZKMetadata() call
##########
File path: pinot-common/src/test/java/org/apache/pinot/common/tier/TierSegmentSelectorTest.java
##########
@@ -46,8 +45,8 @@ public void testTimeBasedSegmentSelector() {
// offline segment
String segmentName = "segment_0";
String tableNameWithType = "myTable_OFFLINE";
- OfflineSegmentZKMetadata offlineSegmentZKMetadata = new OfflineSegmentZKMetadata();
- offlineSegmentZKMetadata.setSegmentName(segmentName);
+ SegmentZKMetadata offlineSegmentZKMetadata = new SegmentZKMetadata(segmentName);
+ offlineSegmentZKMetadata.setSegmentType(CommonConstants.Segment.SegmentType.OFFLINE);
Review comment:
I didn't see setSegmentType() was called in other places above.
Is type required? If so, can it be added to constructor?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java
##########
@@ -36,209 +34,291 @@
import org.slf4j.LoggerFactory;
-public abstract class SegmentZKMetadata implements ZKMetadata {
+public class SegmentZKMetadata implements ZKMetadata {
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentZKMetadata.class);
+ private static final String NULL = "null";
- protected static final String NULL = "null";
-
- private String _segmentName;
- private SegmentType _segmentType;
- private long _startTime = -1;
- private long _endTime = -1;
- private TimeUnit _timeUnit;
- private String _indexVersion;
- private long _totalDocs = -1;
- private long _crc = -1;
- private long _creationTime = -1;
- private SegmentPartitionMetadata _partitionMetadata;
- private long _segmentUploadStartTime = -1;
- private String _crypterName;
- private Map<String, String> _customMap;
+ private final ZNRecord _znRecord;
+ private Map<String, String> _simpleFields;
- @Deprecated
- private String _rawTableName;
-
- public SegmentZKMetadata() {
+ public SegmentZKMetadata(String segmentName) {
+ _znRecord = new ZNRecord(segmentName);
+ _simpleFields = _znRecord.getSimpleFields();
+ // TODO: Remove this field after releasing 0.9.0
Review comment:
would be helpful to also comment the reason to leave this field till 0.9.0
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r685463028
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
##########
@@ -144,34 +141,25 @@ public static void removeResourceConfigFromPropertyStore(ZkHelixPropertyStore<ZN
}
}
- public static boolean setOfflineSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String offlineTableName, OfflineSegmentZKMetadata offlineSegmentZKMetadata, int expectedVersion) {
+ public static boolean setSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore, String tableNameWithType,
+ SegmentZKMetadata segmentZKMetadata, int expectedVersion) {
// NOTE: Helix will throw ZkBadVersionException if version does not match
try {
return propertyStore
- .set(constructPropertyStorePathForSegment(offlineTableName, offlineSegmentZKMetadata.getSegmentName()),
- offlineSegmentZKMetadata.toZNRecord(), expectedVersion, AccessOption.PERSISTENT);
+ .set(constructPropertyStorePathForSegment(tableNameWithType, segmentZKMetadata.getSegmentName()),
+ segmentZKMetadata.toZNRecord(), expectedVersion, AccessOption.PERSISTENT);
} catch (ZkBadVersionException e) {
return false;
}
}
- public static boolean setOfflineSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String offlineTableName, OfflineSegmentZKMetadata offlineSegmentZKMetadata) {
- return propertyStore
- .set(constructPropertyStorePathForSegment(offlineTableName, offlineSegmentZKMetadata.getSegmentName()),
- offlineSegmentZKMetadata.toZNRecord(), AccessOption.PERSISTENT);
- }
-
- public static boolean setRealtimeSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String realtimeTableName, RealtimeSegmentZKMetadata realtimeSegmentZKMetadata) {
- return propertyStore
- .set(constructPropertyStorePathForSegment(realtimeTableName, realtimeSegmentZKMetadata.getSegmentName()),
- realtimeSegmentZKMetadata.toZNRecord(), AccessOption.PERSISTENT);
+ public static boolean setSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore, String tableNameWithType,
+ SegmentZKMetadata segmentZKMetadata) {
+ return setSegmentZKMetadata(propertyStore, tableNameWithType, segmentZKMetadata, -1);
}
@Nullable
- public static ZNRecord getZnRecord(@Nonnull ZkHelixPropertyStore<ZNRecord> propertyStore, @Nonnull String path) {
+ public static ZNRecord getZnRecord(ZkHelixPropertyStore<ZNRecord> propertyStore, String path) {
Review comment:
This annotation is mostly for readability purpose, and will be ignored by the compiler. IDE will perform some simple check and warn when invoking methods on `nullable` values.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
klsince commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r685455743
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
##########
@@ -144,34 +141,25 @@ public static void removeResourceConfigFromPropertyStore(ZkHelixPropertyStore<ZN
}
}
- public static boolean setOfflineSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String offlineTableName, OfflineSegmentZKMetadata offlineSegmentZKMetadata, int expectedVersion) {
+ public static boolean setSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore, String tableNameWithType,
+ SegmentZKMetadata segmentZKMetadata, int expectedVersion) {
// NOTE: Helix will throw ZkBadVersionException if version does not match
try {
return propertyStore
- .set(constructPropertyStorePathForSegment(offlineTableName, offlineSegmentZKMetadata.getSegmentName()),
- offlineSegmentZKMetadata.toZNRecord(), expectedVersion, AccessOption.PERSISTENT);
+ .set(constructPropertyStorePathForSegment(tableNameWithType, segmentZKMetadata.getSegmentName()),
+ segmentZKMetadata.toZNRecord(), expectedVersion, AccessOption.PERSISTENT);
} catch (ZkBadVersionException e) {
return false;
}
}
- public static boolean setOfflineSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String offlineTableName, OfflineSegmentZKMetadata offlineSegmentZKMetadata) {
- return propertyStore
- .set(constructPropertyStorePathForSegment(offlineTableName, offlineSegmentZKMetadata.getSegmentName()),
- offlineSegmentZKMetadata.toZNRecord(), AccessOption.PERSISTENT);
- }
-
- public static boolean setRealtimeSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String realtimeTableName, RealtimeSegmentZKMetadata realtimeSegmentZKMetadata) {
- return propertyStore
- .set(constructPropertyStorePathForSegment(realtimeTableName, realtimeSegmentZKMetadata.getSegmentName()),
- realtimeSegmentZKMetadata.toZNRecord(), AccessOption.PERSISTENT);
+ public static boolean setSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore, String tableNameWithType,
+ SegmentZKMetadata segmentZKMetadata) {
+ return setSegmentZKMetadata(propertyStore, tableNameWithType, segmentZKMetadata, -1);
}
@Nullable
- public static ZNRecord getZnRecord(@Nonnull ZkHelixPropertyStore<ZNRecord> propertyStore, @Nonnull String path) {
+ public static ZNRecord getZnRecord(ZkHelixPropertyStore<ZNRecord> propertyStore, String path) {
Review comment:
ic. I see some methods mark one param as Nullable and leave others without annotation. In this case, does compile assume the other Nonnull or Nullable?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r688105049
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java
##########
@@ -36,209 +34,302 @@
import org.slf4j.LoggerFactory;
-public abstract class SegmentZKMetadata implements ZKMetadata {
+public class SegmentZKMetadata implements ZKMetadata {
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentZKMetadata.class);
+ private static final String NULL = "null";
- protected static final String NULL = "null";
-
- private String _segmentName;
- private SegmentType _segmentType;
- private long _startTime = -1;
- private long _endTime = -1;
- private TimeUnit _timeUnit;
- private String _indexVersion;
- private long _totalDocs = -1;
- private long _crc = -1;
- private long _creationTime = -1;
- private SegmentPartitionMetadata _partitionMetadata;
- private long _segmentUploadStartTime = -1;
- private String _crypterName;
- private Map<String, String> _customMap;
+ private final ZNRecord _znRecord;
+ private Map<String, String> _simpleFields;
- @Deprecated
- private String _rawTableName;
+ // Cache start/end time because they can be used to sort the metadata
+ private long _startTimeMs = -1;
+ private long _endTimeMs = -1;
- public SegmentZKMetadata() {
+ public SegmentZKMetadata(String segmentName) {
+ _znRecord = new ZNRecord(segmentName);
+ _simpleFields = _znRecord.getSimpleFields();
+ // TODO: Remove this field after releasing 0.9.0
+ _simpleFields.put(Segment.SEGMENT_NAME, segmentName);
}
public SegmentZKMetadata(ZNRecord znRecord) {
- _segmentName = znRecord.getSimpleField(Segment.SEGMENT_NAME);
- _segmentType = znRecord.getEnumField(Segment.SEGMENT_TYPE, SegmentType.class, SegmentType.OFFLINE);
- _startTime = znRecord.getLongField(Segment.START_TIME, -1);
- _endTime = znRecord.getLongField(Segment.END_TIME, -1);
- String timeUnitString = znRecord.getSimpleField(Segment.TIME_UNIT);
- if (timeUnitString != null && !timeUnitString.equals(NULL)) {
- _timeUnit = znRecord.getEnumField(Segment.TIME_UNIT, TimeUnit.class, TimeUnit.DAYS);
- }
- _indexVersion = znRecord.getSimpleField(Segment.INDEX_VERSION);
- _totalDocs = znRecord.getLongField(Segment.TOTAL_DOCS, -1);
- _crc = znRecord.getLongField(Segment.CRC, -1);
- _creationTime = znRecord.getLongField(Segment.CREATION_TIME, -1);
- try {
- String partitionMetadataJson = znRecord.getSimpleField(Segment.PARTITION_METADATA);
- if (partitionMetadataJson != null) {
- _partitionMetadata = SegmentPartitionMetadata.fromJsonString(partitionMetadataJson);
- }
- } catch (IOException e) {
- LOGGER.error(
- "Exception caught while reading partition info from zk metadata for segment '{}', partition info dropped.",
- _segmentName, e);
- }
- _segmentUploadStartTime = znRecord.getLongField(Segment.SEGMENT_UPLOAD_START_TIME, -1);
- _crypterName = znRecord.getSimpleField(Segment.CRYPTER_NAME);
- _customMap = znRecord.getMapField(Segment.CUSTOM_MAP);
-
- // For backward-compatibility
- setTableName(znRecord.getSimpleField(Segment.TABLE_NAME));
+ _znRecord = znRecord;
+ _simpleFields = znRecord.getSimpleFields();
}
public String getSegmentName() {
- return _segmentName;
- }
-
- public void setSegmentName(String segmentName) {
- _segmentName = segmentName;
- }
-
- public SegmentType getSegmentType() {
- return _segmentType;
- }
-
- public void setSegmentType(SegmentType segmentType) {
- _segmentType = segmentType;
+ return _znRecord.getId();
}
public long getStartTimeMs() {
- if (_startTime > 0 && _timeUnit != null) {
- return _timeUnit.toMillis(_startTime);
- } else {
- return -1;
+ if (_startTimeMs < 0) {
Review comment:
Added a boolean field to track whether the time is cached
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r688103149
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java
##########
@@ -377,10 +377,9 @@ public void run() {
try {
_segmentLogger.info("Marking current segment as completed in Helix");
- RealtimeSegmentZKMetadata metadataToOverwrite = new RealtimeSegmentZKMetadata();
+ SegmentZKMetadata metadataToOverwrite = new SegmentZKMetadata(segmentZKMetadata.getSegmentName());
metadataToOverwrite.setTableName(_tableNameWithType);
- metadataToOverwrite.setSegmentName(realtimeSegmentZKMetadata.getSegmentName());
- metadataToOverwrite.setSegmentType(SegmentType.OFFLINE);
Review comment:
I'll keep it the same as before in case it is used (I don't see any usage though).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r686340364
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java
##########
@@ -136,14 +135,14 @@ static long computeRealtimeTotalDocumentInSegments(List<RealtimeSegmentZKMetadat
groupId = segmentGroupIdName;
}
// Discard all segments with different groupids as they are replicas
- if (groupId.equals(segmentGroupIdName) && realtimeSegmentZKMetadata.getTotalDocs() >= 0) {
- numTotalDocs += realtimeSegmentZKMetadata.getTotalDocs();
+ if (groupId.equals(segmentGroupIdName) && segmentZKMetadata.getTotalDocs() >= 0) {
+ numTotalDocs += segmentZKMetadata.getTotalDocs();
}
}
} else {
Review comment:
I think it is needed during the migration from HLC to LLC. Also, out of the scope of this PR
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r686343622
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java
##########
@@ -377,10 +377,9 @@ public void run() {
try {
_segmentLogger.info("Marking current segment as completed in Helix");
- RealtimeSegmentZKMetadata metadataToOverwrite = new RealtimeSegmentZKMetadata();
+ SegmentZKMetadata metadataToOverwrite = new SegmentZKMetadata(segmentZKMetadata.getSegmentName());
metadataToOverwrite.setTableName(_tableNameWithType);
- metadataToOverwrite.setSegmentName(realtimeSegmentZKMetadata.getSegmentName());
- metadataToOverwrite.setSegmentType(SegmentType.OFFLINE);
Review comment:
Seems a typo to me. It does not matter actually because segment type is already deprecated and is only used to determine the key for download url, which does not apply to the HLC segment.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #7255:
URL: https://github.com/apache/pinot/pull/7255
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r686342060
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java
##########
@@ -90,7 +91,7 @@
static final long START_TIME_MS = CURRENT_TIME_MS - TimeUnit.HOURS.toMillis(RANDOM.nextInt(24) + 24);
static final long END_TIME_MS = START_TIME_MS + TimeUnit.HOURS.toMillis(RANDOM.nextInt(24) + 1);
static final Interval INTERVAL = new Interval(START_TIME_MS, END_TIME_MS);
- static final String CRC = Long.toString(RANDOM.nextLong());
+ static final String CRC = Long.toString(RANDOM.nextLong() & 0xFFFFFFFFL);
Review comment:
Added comment: CRC is always non-negative
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jtao15 commented on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
jtao15 commented on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-898024202
@Jackie-Jiang We have the purge task scheduler in our internal code bases which fetch segment zk metadata from `ClusterInfoAccessor`.
@snleee I'll update our internal code once this pr is merged.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jtao15 commented on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
jtao15 commented on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-898039341
LGTM
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-893851344
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7255](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b9cec77) into [master](https://codecov.io/gh/apache/pinot/commit/44f31ac8fe70947144fe383c44d660128e7952b7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (44f31ac) will **decrease** coverage by `2.20%`.
> The diff coverage is `36.09%`.
[](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7255 +/- ##
============================================
- Coverage 71.23% 69.02% -2.21%
+ Complexity 3273 3185 -88
============================================
Files 1505 1108 -397
Lines 74224 52127 -22097
Branches 10733 7789 -2944
============================================
- Hits 52872 35980 -16892
+ Misses 17750 13571 -4179
+ Partials 3602 2576 -1026
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `69.02% <36.09%> (+0.07%)` | :arrow_up: |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/common/utils/SegmentUtils.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvU2VnbWVudFV0aWxzLmphdmE=) | `0.00% <0.00%> (-68.75%)` | :arrow_down: |
| [...manager/realtime/HLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvSExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `0.00% <0.00%> (-83.03%)` | :arrow_down: |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `10.61% <0.00%> (-56.47%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `25.00% <ø> (ø)` | |
| [...ache/pinot/common/metadata/ZKMetadataProvider.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0YWRhdGEvWktNZXRhZGF0YVByb3ZpZGVyLmphdmE=) | `12.03% <7.69%> (-64.61%)` | :arrow_down: |
| [...not/common/metadata/segment/SegmentZKMetadata.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0YWRhdGEvc2VnbWVudC9TZWdtZW50WktNZXRhZGF0YS5qYXZh) | `44.44% <38.40%> (-31.56%)` | :arrow_down: |
| [...manager/realtime/LLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvTExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `47.26% <57.14%> (-25.60%)` | :arrow_down: |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `58.26% <87.50%> (-0.09%)` | :arrow_down: |
| [...inot/common/tier/TimeBasedTierSegmentSelector.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaW1lQmFzZWRUaWVyU2VnbWVudFNlbGVjdG9yLmphdmE=) | `94.44% <100.00%> (ø)` | |
| [...ent/local/realtime/impl/RealtimeSegmentConfig.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL1JlYWx0aW1lU2VnbWVudENvbmZpZy5qYXZh) | `91.86% <100.00%> (ø)` | |
| ... and [575 more](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [44f31ac...b9cec77](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r688102332
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1742,37 +1736,36 @@ public ZNRecord getSegmentMetadataZnRecord(String tableNameWithType, String segm
ZKMetadataProvider.constructPropertyStorePathForSegment(tableNameWithType, segmentName));
}
- public boolean updateZkMetadata(String offlineTableName, OfflineSegmentZKMetadata segmentMetadata,
- int expectedVersion) {
+ public boolean updateZkMetadata(String tableNameWithType, SegmentZKMetadata segmentZKMetadata, int expectedVersion) {
return ZKMetadataProvider
- .setOfflineSegmentZKMetadata(_propertyStore, offlineTableName, segmentMetadata, expectedVersion);
+ .setSegmentZKMetadata(_propertyStore, tableNameWithType, segmentZKMetadata, expectedVersion);
}
- public boolean updateZkMetadata(String offlineTableName, OfflineSegmentZKMetadata segmentMetadata) {
- return ZKMetadataProvider.setOfflineSegmentZKMetadata(_propertyStore, offlineTableName, segmentMetadata);
+ public boolean updateZkMetadata(String tableNameWithType, SegmentZKMetadata segmentZKMetadata) {
+ return ZKMetadataProvider.setSegmentZKMetadata(_propertyStore, tableNameWithType, segmentZKMetadata);
}
- public void refreshSegment(String offlineTableName, SegmentMetadata segmentMetadata,
- OfflineSegmentZKMetadata offlineSegmentZKMetadata, String downloadUrl, @Nullable String crypter) {
+ public void refreshSegment(String tableNameWithType, SegmentMetadata segmentMetadata,
+ SegmentZKMetadata segmentZKMetadata, String downloadUrl, @Nullable String crypter) {
String segmentName = segmentMetadata.getName();
// NOTE: Must first set the segment ZK metadata before trying to refresh because servers and brokers rely on segment
// ZK metadata to refresh the segment (server will compare the segment ZK metadata with the local metadata to decide
// whether to download the new segment; broker will update the the segment partition info & time boundary based on
// the segment ZK metadata)
ZKMetadataUtils
- .updateSegmentMetadata(offlineSegmentZKMetadata, segmentMetadata, CommonConstants.Segment.SegmentType.OFFLINE);
- offlineSegmentZKMetadata.setRefreshTime(System.currentTimeMillis());
- offlineSegmentZKMetadata.setDownloadUrl(downloadUrl);
- offlineSegmentZKMetadata.setCrypterName(crypter);
- if (!ZKMetadataProvider.setOfflineSegmentZKMetadata(_propertyStore, offlineTableName, offlineSegmentZKMetadata)) {
+ .updateSegmentMetadata(segmentZKMetadata, segmentMetadata, CommonConstants.Segment.SegmentType.OFFLINE);
Review comment:
For now, yes. When we remove the segment type completely after releasing `0.9.0`, we can remove it.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] snleee edited a comment on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
snleee edited a comment on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-897996764
@Jackie-Jiang
This will break the external code-bases on the compilation time when they have used `OfflineSegmentZKMetadata, RealtimeSegmentZkMetadata, etc`. One example @ Linkedin would be the purge task generator where we scan the segments. Please add the relevant label (backward-incompat, release-note).
@jtao15 This change will break the release pipeline. We will need to update our internal code bases to reflect this change. (e.g. `OfflineSegmentZkMetadata -> SegmentZkMetadata`).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] snleee commented on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
snleee commented on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-897996764
This will break the external code-base from compilation when they have used `OfflineSegmentZKMetadata, RealtimeSegmentZkMetadata, etc`. One example @ Linkedin would be the purge task generator where we scan the segments. Please add the relevant label (backward-incompat, release-note).
@jtao15 This change will break the release pipeline. We will need to update our internal code bases to reflect this change. (e.g. `OfflineSegmentZkMetadata -> SegmentZkMetadata`).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
klsince commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r685368715
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
##########
@@ -144,34 +141,25 @@ public static void removeResourceConfigFromPropertyStore(ZkHelixPropertyStore<ZN
}
}
- public static boolean setOfflineSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String offlineTableName, OfflineSegmentZKMetadata offlineSegmentZKMetadata, int expectedVersion) {
+ public static boolean setSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore, String tableNameWithType,
+ SegmentZKMetadata segmentZKMetadata, int expectedVersion) {
// NOTE: Helix will throw ZkBadVersionException if version does not match
try {
return propertyStore
- .set(constructPropertyStorePathForSegment(offlineTableName, offlineSegmentZKMetadata.getSegmentName()),
- offlineSegmentZKMetadata.toZNRecord(), expectedVersion, AccessOption.PERSISTENT);
+ .set(constructPropertyStorePathForSegment(tableNameWithType, segmentZKMetadata.getSegmentName()),
+ segmentZKMetadata.toZNRecord(), expectedVersion, AccessOption.PERSISTENT);
} catch (ZkBadVersionException e) {
return false;
}
}
- public static boolean setOfflineSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String offlineTableName, OfflineSegmentZKMetadata offlineSegmentZKMetadata) {
- return propertyStore
- .set(constructPropertyStorePathForSegment(offlineTableName, offlineSegmentZKMetadata.getSegmentName()),
- offlineSegmentZKMetadata.toZNRecord(), AccessOption.PERSISTENT);
- }
-
- public static boolean setRealtimeSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
- String realtimeTableName, RealtimeSegmentZKMetadata realtimeSegmentZKMetadata) {
- return propertyStore
- .set(constructPropertyStorePathForSegment(realtimeTableName, realtimeSegmentZKMetadata.getSegmentName()),
- realtimeSegmentZKMetadata.toZNRecord(), AccessOption.PERSISTENT);
+ public static boolean setSegmentZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore, String tableNameWithType,
+ SegmentZKMetadata segmentZKMetadata) {
+ return setSegmentZKMetadata(propertyStore, tableNameWithType, segmentZKMetadata, -1);
}
@Nullable
- public static ZNRecord getZnRecord(@Nonnull ZkHelixPropertyStore<ZNRecord> propertyStore, @Nonnull String path) {
+ public static ZNRecord getZnRecord(ZkHelixPropertyStore<ZNRecord> propertyStore, String path) {
Review comment:
why removing all @Nonnull annotation?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] snleee edited a comment on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
snleee edited a comment on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-897996764
@Jackie-Jiang
This will break the external code-base from compilation when they have used `OfflineSegmentZKMetadata, RealtimeSegmentZkMetadata, etc`. One example @ Linkedin would be the purge task generator where we scan the segments. Please add the relevant label (backward-incompat, release-note).
@jtao15 This change will break the release pipeline. We will need to update our internal code bases to reflect this change. (e.g. `OfflineSegmentZkMetadata -> SegmentZkMetadata`).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-893851344
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7255](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fa6a221) into [master](https://codecov.io/gh/apache/pinot/commit/066c79399922fecc428ff05b2b983ef7367512d3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (066c793) will **decrease** coverage by `8.17%`.
> The diff coverage is `61.47%`.
[](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7255 +/- ##
============================================
- Coverage 73.53% 65.35% -8.18%
Complexity 92 92
============================================
Files 1508 1505 -3
Lines 73999 73761 -238
Branches 10691 10668 -23
============================================
- Hits 54413 48208 -6205
- Misses 16020 22138 +6118
+ Partials 3566 3415 -151
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.35% <61.47%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...troller/helix/core/minion/ClusterInfoAccessor.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9DbHVzdGVySW5mb0FjY2Vzc29yLmphdmE=) | `39.13% <0.00%> (-32.30%)` | :arrow_down: |
| [...realtime/segment/DefaultFlushThresholdUpdater.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL3NlZ21lbnQvRGVmYXVsdEZsdXNoVGhyZXNob2xkVXBkYXRlci5qYXZh) | `100.00% <ø> (ø)` | |
| [...segment/SegmentSizeBasedFlushThresholdUpdater.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL3NlZ21lbnQvU2VnbWVudFNpemVCYXNlZEZsdXNoVGhyZXNob2xkVXBkYXRlci5qYXZh) | `70.58% <ø> (ø)` | |
| [...pinot/controller/util/TableRetentionValidator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL1RhYmxlUmV0ZW50aW9uVmFsaWRhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...ller/validation/OfflineSegmentIntervalChecker.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL09mZmxpbmVTZWdtZW50SW50ZXJ2YWxDaGVja2VyLmphdmE=) | `29.11% <0.00%> (-57.14%)` | :arrow_down: |
| [...manager/realtime/HLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvSExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `0.00% <0.00%> (-83.03%)` | :arrow_down: |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `10.65% <0.00%> (-56.29%)` | :arrow_down: |
| [...he/pinot/plugin/minion/tasks/BaseTaskExecutor.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvQmFzZVRhc2tFeGVjdXRvci5qYXZh) | `47.05% <0.00%> (-29.42%)` | :arrow_down: |
| [...t\_to\_raw\_index/ConvertToRawIndexTaskGenerator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvY29udmVydF90b19yYXdfaW5kZXgvQ29udmVydFRvUmF3SW5kZXhUYXNrR2VuZXJhdG9yLmphdmE=) | `10.41% <0.00%> (-77.34%)` | :arrow_down: |
| [...nd\_push/SegmentGenerationAndPushTaskGenerator.java](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3Mvc2VnbWVudF9nZW5lcmF0aW9uX2FuZF9wdXNoL1NlZ21lbnRHZW5lcmF0aW9uQW5kUHVzaFRhc2tHZW5lcmF0b3IuamF2YQ==) | `8.49% <0.00%> (-0.06%)` | :arrow_down: |
| ... and [386 more](https://codecov.io/gh/apache/pinot/pull/7255/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [066c793...fa6a221](https://codecov.io/gh/apache/pinot/pull/7255?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#issuecomment-898002428
@snleee Added the labels and the release note. The `SegmentZKMetadata` is not meant to be used by the external code, but the task scheduler might need to access them.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r687269751
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1742,37 +1736,36 @@ public ZNRecord getSegmentMetadataZnRecord(String tableNameWithType, String segm
ZKMetadataProvider.constructPropertyStorePathForSegment(tableNameWithType, segmentName));
}
- public boolean updateZkMetadata(String offlineTableName, OfflineSegmentZKMetadata segmentMetadata,
- int expectedVersion) {
+ public boolean updateZkMetadata(String tableNameWithType, SegmentZKMetadata segmentZKMetadata, int expectedVersion) {
return ZKMetadataProvider
- .setOfflineSegmentZKMetadata(_propertyStore, offlineTableName, segmentMetadata, expectedVersion);
+ .setSegmentZKMetadata(_propertyStore, tableNameWithType, segmentZKMetadata, expectedVersion);
}
- public boolean updateZkMetadata(String offlineTableName, OfflineSegmentZKMetadata segmentMetadata) {
- return ZKMetadataProvider.setOfflineSegmentZKMetadata(_propertyStore, offlineTableName, segmentMetadata);
+ public boolean updateZkMetadata(String tableNameWithType, SegmentZKMetadata segmentZKMetadata) {
+ return ZKMetadataProvider.setSegmentZKMetadata(_propertyStore, tableNameWithType, segmentZKMetadata);
}
- public void refreshSegment(String offlineTableName, SegmentMetadata segmentMetadata,
- OfflineSegmentZKMetadata offlineSegmentZKMetadata, String downloadUrl, @Nullable String crypter) {
+ public void refreshSegment(String tableNameWithType, SegmentMetadata segmentMetadata,
+ SegmentZKMetadata segmentZKMetadata, String downloadUrl, @Nullable String crypter) {
String segmentName = segmentMetadata.getName();
// NOTE: Must first set the segment ZK metadata before trying to refresh because servers and brokers rely on segment
// ZK metadata to refresh the segment (server will compare the segment ZK metadata with the local metadata to decide
// whether to download the new segment; broker will update the the segment partition info & time boundary based on
// the segment ZK metadata)
ZKMetadataUtils
- .updateSegmentMetadata(offlineSegmentZKMetadata, segmentMetadata, CommonConstants.Segment.SegmentType.OFFLINE);
- offlineSegmentZKMetadata.setRefreshTime(System.currentTimeMillis());
- offlineSegmentZKMetadata.setDownloadUrl(downloadUrl);
- offlineSegmentZKMetadata.setCrypterName(crypter);
- if (!ZKMetadataProvider.setOfflineSegmentZKMetadata(_propertyStore, offlineTableName, offlineSegmentZKMetadata)) {
+ .updateSegmentMetadata(segmentZKMetadata, segmentMetadata, CommonConstants.Segment.SegmentType.OFFLINE);
Review comment:
do we still need segment type here?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java
##########
@@ -36,209 +34,302 @@
import org.slf4j.LoggerFactory;
-public abstract class SegmentZKMetadata implements ZKMetadata {
+public class SegmentZKMetadata implements ZKMetadata {
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentZKMetadata.class);
+ private static final String NULL = "null";
- protected static final String NULL = "null";
-
- private String _segmentName;
- private SegmentType _segmentType;
- private long _startTime = -1;
- private long _endTime = -1;
- private TimeUnit _timeUnit;
- private String _indexVersion;
- private long _totalDocs = -1;
- private long _crc = -1;
- private long _creationTime = -1;
- private SegmentPartitionMetadata _partitionMetadata;
- private long _segmentUploadStartTime = -1;
- private String _crypterName;
- private Map<String, String> _customMap;
+ private final ZNRecord _znRecord;
+ private Map<String, String> _simpleFields;
- @Deprecated
- private String _rawTableName;
+ // Cache start/end time because they can be used to sort the metadata
+ private long _startTimeMs = -1;
+ private long _endTimeMs = -1;
- public SegmentZKMetadata() {
+ public SegmentZKMetadata(String segmentName) {
+ _znRecord = new ZNRecord(segmentName);
+ _simpleFields = _znRecord.getSimpleFields();
+ // TODO: Remove this field after releasing 0.9.0
+ _simpleFields.put(Segment.SEGMENT_NAME, segmentName);
}
public SegmentZKMetadata(ZNRecord znRecord) {
- _segmentName = znRecord.getSimpleField(Segment.SEGMENT_NAME);
- _segmentType = znRecord.getEnumField(Segment.SEGMENT_TYPE, SegmentType.class, SegmentType.OFFLINE);
- _startTime = znRecord.getLongField(Segment.START_TIME, -1);
- _endTime = znRecord.getLongField(Segment.END_TIME, -1);
- String timeUnitString = znRecord.getSimpleField(Segment.TIME_UNIT);
- if (timeUnitString != null && !timeUnitString.equals(NULL)) {
- _timeUnit = znRecord.getEnumField(Segment.TIME_UNIT, TimeUnit.class, TimeUnit.DAYS);
- }
- _indexVersion = znRecord.getSimpleField(Segment.INDEX_VERSION);
- _totalDocs = znRecord.getLongField(Segment.TOTAL_DOCS, -1);
- _crc = znRecord.getLongField(Segment.CRC, -1);
- _creationTime = znRecord.getLongField(Segment.CREATION_TIME, -1);
- try {
- String partitionMetadataJson = znRecord.getSimpleField(Segment.PARTITION_METADATA);
- if (partitionMetadataJson != null) {
- _partitionMetadata = SegmentPartitionMetadata.fromJsonString(partitionMetadataJson);
- }
- } catch (IOException e) {
- LOGGER.error(
- "Exception caught while reading partition info from zk metadata for segment '{}', partition info dropped.",
- _segmentName, e);
- }
- _segmentUploadStartTime = znRecord.getLongField(Segment.SEGMENT_UPLOAD_START_TIME, -1);
- _crypterName = znRecord.getSimpleField(Segment.CRYPTER_NAME);
- _customMap = znRecord.getMapField(Segment.CUSTOM_MAP);
-
- // For backward-compatibility
- setTableName(znRecord.getSimpleField(Segment.TABLE_NAME));
+ _znRecord = znRecord;
+ _simpleFields = znRecord.getSimpleFields();
}
public String getSegmentName() {
- return _segmentName;
- }
-
- public void setSegmentName(String segmentName) {
- _segmentName = segmentName;
- }
-
- public SegmentType getSegmentType() {
- return _segmentType;
- }
-
- public void setSegmentType(SegmentType segmentType) {
- _segmentType = segmentType;
+ return _znRecord.getId();
}
public long getStartTimeMs() {
- if (_startTime > 0 && _timeUnit != null) {
- return _timeUnit.toMillis(_startTime);
- } else {
- return -1;
+ if (_startTimeMs < 0) {
Review comment:
use <= ?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java
##########
@@ -377,10 +377,9 @@ public void run() {
try {
_segmentLogger.info("Marking current segment as completed in Helix");
- RealtimeSegmentZKMetadata metadataToOverwrite = new RealtimeSegmentZKMetadata();
+ SegmentZKMetadata metadataToOverwrite = new SegmentZKMetadata(segmentZKMetadata.getSegmentName());
metadataToOverwrite.setTableName(_tableNameWithType);
- metadataToOverwrite.setSegmentName(realtimeSegmentZKMetadata.getSegmentName());
- metadataToOverwrite.setSegmentType(SegmentType.OFFLINE);
Review comment:
I think we used to claim only consuming segment as real-time segment and all persisted(completed) segments are in `OFFLINE` type.
If we don't have any logic rely on this, then it should be fine. E.g. segment retention.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java
##########
@@ -36,209 +34,302 @@
import org.slf4j.LoggerFactory;
-public abstract class SegmentZKMetadata implements ZKMetadata {
+public class SegmentZKMetadata implements ZKMetadata {
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentZKMetadata.class);
+ private static final String NULL = "null";
- protected static final String NULL = "null";
-
- private String _segmentName;
- private SegmentType _segmentType;
- private long _startTime = -1;
- private long _endTime = -1;
- private TimeUnit _timeUnit;
- private String _indexVersion;
- private long _totalDocs = -1;
- private long _crc = -1;
- private long _creationTime = -1;
- private SegmentPartitionMetadata _partitionMetadata;
- private long _segmentUploadStartTime = -1;
- private String _crypterName;
- private Map<String, String> _customMap;
+ private final ZNRecord _znRecord;
+ private Map<String, String> _simpleFields;
- @Deprecated
- private String _rawTableName;
+ // Cache start/end time because they can be used to sort the metadata
+ private long _startTimeMs = -1;
+ private long _endTimeMs = -1;
- public SegmentZKMetadata() {
+ public SegmentZKMetadata(String segmentName) {
+ _znRecord = new ZNRecord(segmentName);
+ _simpleFields = _znRecord.getSimpleFields();
+ // TODO: Remove this field after releasing 0.9.0
+ _simpleFields.put(Segment.SEGMENT_NAME, segmentName);
}
public SegmentZKMetadata(ZNRecord znRecord) {
- _segmentName = znRecord.getSimpleField(Segment.SEGMENT_NAME);
- _segmentType = znRecord.getEnumField(Segment.SEGMENT_TYPE, SegmentType.class, SegmentType.OFFLINE);
- _startTime = znRecord.getLongField(Segment.START_TIME, -1);
- _endTime = znRecord.getLongField(Segment.END_TIME, -1);
- String timeUnitString = znRecord.getSimpleField(Segment.TIME_UNIT);
- if (timeUnitString != null && !timeUnitString.equals(NULL)) {
- _timeUnit = znRecord.getEnumField(Segment.TIME_UNIT, TimeUnit.class, TimeUnit.DAYS);
- }
- _indexVersion = znRecord.getSimpleField(Segment.INDEX_VERSION);
- _totalDocs = znRecord.getLongField(Segment.TOTAL_DOCS, -1);
- _crc = znRecord.getLongField(Segment.CRC, -1);
- _creationTime = znRecord.getLongField(Segment.CREATION_TIME, -1);
- try {
- String partitionMetadataJson = znRecord.getSimpleField(Segment.PARTITION_METADATA);
- if (partitionMetadataJson != null) {
- _partitionMetadata = SegmentPartitionMetadata.fromJsonString(partitionMetadataJson);
- }
- } catch (IOException e) {
- LOGGER.error(
- "Exception caught while reading partition info from zk metadata for segment '{}', partition info dropped.",
- _segmentName, e);
- }
- _segmentUploadStartTime = znRecord.getLongField(Segment.SEGMENT_UPLOAD_START_TIME, -1);
- _crypterName = znRecord.getSimpleField(Segment.CRYPTER_NAME);
- _customMap = znRecord.getMapField(Segment.CUSTOM_MAP);
-
- // For backward-compatibility
- setTableName(znRecord.getSimpleField(Segment.TABLE_NAME));
+ _znRecord = znRecord;
+ _simpleFields = znRecord.getSimpleFields();
}
public String getSegmentName() {
- return _segmentName;
- }
-
- public void setSegmentName(String segmentName) {
- _segmentName = segmentName;
- }
-
- public SegmentType getSegmentType() {
- return _segmentType;
- }
-
- public void setSegmentType(SegmentType segmentType) {
- _segmentType = segmentType;
+ return _znRecord.getId();
}
public long getStartTimeMs() {
- if (_startTime > 0 && _timeUnit != null) {
- return _timeUnit.toMillis(_startTime);
- } else {
- return -1;
+ if (_startTimeMs < 0) {
+ String startTimeString = _simpleFields.get(Segment.START_TIME);
+ if (startTimeString != null) {
+ _startTimeMs = TimeUnit.valueOf(_simpleFields.get(Segment.TIME_UNIT)).toMillis(Long.parseLong(startTimeString));
+ }
}
- }
-
- public void setStartTime(long startTime) {
- _startTime = startTime;
+ return _startTimeMs;
}
public long getEndTimeMs() {
- if (_endTime > 0 && _timeUnit != null) {
- return _timeUnit.toMillis(_endTime);
- } else {
- return -1;
+ if (_endTimeMs < 0) {
Review comment:
use <= ?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7255: Unify segment ZK metadata
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r686334111
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java
##########
@@ -36,209 +34,291 @@
import org.slf4j.LoggerFactory;
-public abstract class SegmentZKMetadata implements ZKMetadata {
+public class SegmentZKMetadata implements ZKMetadata {
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentZKMetadata.class);
+ private static final String NULL = "null";
- protected static final String NULL = "null";
-
- private String _segmentName;
- private SegmentType _segmentType;
- private long _startTime = -1;
- private long _endTime = -1;
- private TimeUnit _timeUnit;
- private String _indexVersion;
- private long _totalDocs = -1;
- private long _crc = -1;
- private long _creationTime = -1;
- private SegmentPartitionMetadata _partitionMetadata;
- private long _segmentUploadStartTime = -1;
- private String _crypterName;
- private Map<String, String> _customMap;
+ private final ZNRecord _znRecord;
+ private Map<String, String> _simpleFields;
- @Deprecated
- private String _rawTableName;
-
- public SegmentZKMetadata() {
+ public SegmentZKMetadata(String segmentName) {
+ _znRecord = new ZNRecord(segmentName);
+ _simpleFields = _znRecord.getSimpleFields();
+ // TODO: Remove this field after releasing 0.9.0
Review comment:
I think this comment implies that the field is deprecated and we want to keep backward-compatibility
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org