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 2022/04/11 06:26:52 UTC
[GitHub] [pinot] saurabhd336 opened a new pull request, #8502: Add zk metadata to segment's metadata.properties file
saurabhd336 opened a new pull request, #8502:
URL: https://github.com/apache/pinot/pull/8502
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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 #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8502:
URL: https://github.com/apache/pinot/pull/8502#issuecomment-1094814279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8502?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 [#8502](https://codecov.io/gh/apache/pinot/pull/8502?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0bb18ed) into [master](https://codecov.io/gh/apache/pinot/commit/9314e600808c378663de99a2cdce4d5542ea99df?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9314e60) will **increase** coverage by `5.26%`.
> The diff coverage is `16.66%`.
> :exclamation: Current head 0bb18ed differs from pull request most recent head 32b74df. Consider uploading reports for the commit 32b74df to get more accurate results
```diff
@@ Coverage Diff @@
## master #8502 +/- ##
============================================
+ Coverage 29.25% 34.52% +5.26%
- Complexity 0 84 +84
============================================
Files 1660 1673 +13
Lines 87126 87499 +373
Branches 13203 13242 +39
============================================
+ Hits 25492 30209 +4717
+ Misses 59316 54772 -4544
- Partials 2318 2518 +200
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `26.01% <16.66%> (+0.18%)` | :arrow_up: |
| unittests2 | `14.12% <0.00%> (?)` | |
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/8502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...l/realtime/converter/RealtimeSegmentConverter.java](https://codecov.io/gh/apache/pinot/pull/8502/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvUmVhbHRpbWVTZWdtZW50Q29udmVydGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/8502/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ot/segment/spi/creator/SegmentGeneratorConfig.java](https://codecov.io/gh/apache/pinot/pull/8502/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-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2NyZWF0b3IvU2VnbWVudEdlbmVyYXRvckNvbmZpZy5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...inot/spi/config/table/SegmentZKMetadataConfig.java](https://codecov.io/gh/apache/pinot/pull/8502/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1NlZ21lbnRaS01ldGFkYXRhQ29uZmlnLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...manager/realtime/HLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/8502/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) | `82.79% <100.00%> (ø)` | |
| [...manager/realtime/LLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/8502/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) | `56.91% <100.00%> (-4.75%)` | :arrow_down: |
| [...apache/pinot/common/lineage/LineageEntryState.java](https://codecov.io/gh/apache/pinot/pull/8502/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9MaW5lYWdlRW50cnlTdGF0ZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/pinot/pull/8502/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...nverttorawindex/ConvertToRawIndexTaskExecutor.java](https://codecov.io/gh/apache/pinot/pull/8502/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-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvY29udmVydHRvcmF3aW5kZXgvQ29udmVydFRvUmF3SW5kZXhUYXNrRXhlY3V0b3IuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...e/pinot/common/minion/MergeRollupTaskMetadata.java](https://codecov.io/gh/apache/pinot/pull/8502/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL01lcmdlUm9sbHVwVGFza01ldGFkYXRhLmphdmE=) | `0.00% <0.00%> (-94.74%)` | :arrow_down: |
| ... and [334 more](https://codecov.io/gh/apache/pinot/pull/8502/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/8502?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/8502?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 [9314e60...32b74df](https://codecov.io/gh/apache/pinot/pull/8502?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] saurabhd336 commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r849082596
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -619,6 +621,12 @@ private void writeMetadata()
_dictionaryCreatorMap.containsKey(column), dictionaryElementSize);
}
+ SegmentZKPropsConfig segmentZKMetadataConfig = _config.getSegmentZKPropsConfig();
Review Comment:
Ack
--
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] KKcorps commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r846999015
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentZKMetadataConfig.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+// TODO Possibly redundant class. Use SegmentZKMetadata throughout?
Review Comment:
Any Blocker for doing this?
--
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] saurabhd336 commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r847583488
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentZKMetadataConfig.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+// TODO Possibly redundant class. Use SegmentZKMetadata throughout?
Review Comment:
Ack. Indeed its a rather non trivial refactor
--
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] saurabhd336 commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r847349563
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentZKMetadataConfig.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+// TODO Possibly redundant class. Use SegmentZKMetadata throughout?
Review Comment:
Adding to this, segmentZkMeta is not guaranteed to have endOffset during segment creation (only added during segment commit step). Looks like this class would be needed in that case.
--
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] saurabhd336 commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r848031959
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentZKMetadataConfig.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+// TODO Possibly redundant class. Use SegmentZKMetadata throughout?
+/**
+ * ZK properties that are to be logged into segment's metadata.properties
+ */
+public class SegmentZKMetadataConfig {
Review Comment:
Renamed to SegmentZKPropsConfig. Since these are segment properties. We can use this class to add more ZK props that are to be logged.
--
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] npawar merged pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
npawar merged PR #8502:
URL: https://github.com/apache/pinot/pull/8502
--
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] saurabhd336 commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r849083233
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/RealtimeSegmentConverter.java:
##########
@@ -53,11 +55,13 @@ public class RealtimeSegmentConverter {
private final List<String> _varLengthDictionaryColumns;
private final boolean _nullHandlingEnabled;
- public RealtimeSegmentConverter(MutableSegmentImpl realtimeSegment, String outputPath, Schema schema,
- String tableName, TableConfig tableConfig, String segmentName, String sortedColumn,
- List<String> invertedIndexColumns, List<String> textIndexColumns, List<String> fstIndexColumns,
- List<String> noDictionaryColumns, List<String> varLengthDictionaryColumns, boolean nullHandlingEnabled) {
+ public RealtimeSegmentConverter(MutableSegmentImpl realtimeSegment, SegmentZKPropsConfig segmentZKMetadataConfig,
+ String outputPath, Schema schema, String tableName, TableConfig tableConfig, String segmentName,
+ String sortedColumn, List<String> invertedIndexColumns, List<String> textIndexColumns,
+ List<String> fstIndexColumns, List<String> noDictionaryColumns, List<String> varLengthDictionaryColumns,
+ boolean nullHandlingEnabled) {
_realtimeSegmentImpl = realtimeSegment;
+ _segmentZKMetadataConfig = segmentZKMetadataConfig;
Review Comment:
Ack
--
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] saurabhd336 commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r847582952
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java:
##########
@@ -285,11 +286,13 @@ public void run() {
_segmentLogger.info("Indexed {} raw events", _realtimeSegment.getNumDocsIndexed());
File tempSegmentFolder = new File(_resourceTmpDir, "tmp-" + System.currentTimeMillis());
+ SegmentZKMetadataConfig segmentZKMetadataConfig = new SegmentZKMetadataConfig();
Review Comment:
Passing the SegmentZKMetadataConfig is still needed though.. Should I pass null instead?
--
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] KKcorps commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r847692284
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentZKMetadataConfig.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+// TODO Possibly redundant class. Use SegmentZKMetadata throughout?
Review Comment:
cool! Yeah, I guess there are a couple of interface that need to go in `pinot-spi` instead of core or commons. We can take that up another time.
--
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] npawar commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
npawar commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r847567661
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentZKMetadataConfig.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+// TODO Possibly redundant class. Use SegmentZKMetadata throughout?
+/**
+ * ZK properties that are to be logged into segment's metadata.properties
+ */
+public class SegmentZKMetadataConfig {
Review Comment:
nit: this class is a bit confusing with the SegmentZkMetadata. How about something specific like RealtimeOffsetConfig or directly pass in startOffset endOffset for now?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentZKMetadataConfig.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+// TODO Possibly redundant class. Use SegmentZKMetadata throughout?
Review Comment:
that would touch a lot of files, and I don't even think you'll be able to do that without breaking some things along the way. So if at all, attempt that in a separate PR
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java:
##########
@@ -285,11 +286,13 @@ public void run() {
_segmentLogger.info("Indexed {} raw events", _realtimeSegment.getNumDocsIndexed());
File tempSegmentFolder = new File(_resourceTmpDir, "tmp-" + System.currentTimeMillis());
+ SegmentZKMetadataConfig segmentZKMetadataConfig = new SegmentZKMetadataConfig();
Review Comment:
you can skip this change for HLC
--
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] saurabhd336 commented on pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on PR #8502:
URL: https://github.com/apache/pinot/pull/8502#issuecomment-1097585574
@npawar @KKcorps Have addressed the comments. At this moment, I'm unable to tag it to issue https://github.com/apache/pinot/issues/8489
--
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] KKcorps commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r846998399
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/V1Constants.java:
##########
@@ -76,6 +76,12 @@ private V1Constants() {
public static final String SEGMENT_PADDING_CHARACTER = "segment.padding.character";
public static final String CUSTOM_SUBSET = "custom";
+
+ /**
+ * ZK properties
+ */
+ public static final String SEGMENT_START_OFFSET = "segment.start.offset";
Review Comment:
We can use `Segment.Realtime.START_OFFSET` in place of this
--
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] saurabhd336 commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r847584943
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentZKMetadataConfig.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+// TODO Possibly redundant class. Use SegmentZKMetadata throughout?
+/**
+ * ZK properties that are to be logged into segment's metadata.properties
+ */
+public class SegmentZKMetadataConfig {
Review Comment:
I was going with the general idea that more ZK props will need to be logged hence the name. But will think about this and update
--
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] npawar commented on pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
npawar commented on PR #8502:
URL: https://github.com/apache/pinot/pull/8502#issuecomment-1098247847
please take a look at the checkstyle violations in RealtimeSegmentConverterTest. Once the jobs are green we can merge
--
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] npawar commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
npawar commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r848972965
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/RealtimeSegmentConverter.java:
##########
@@ -53,11 +55,13 @@ public class RealtimeSegmentConverter {
private final List<String> _varLengthDictionaryColumns;
private final boolean _nullHandlingEnabled;
- public RealtimeSegmentConverter(MutableSegmentImpl realtimeSegment, String outputPath, Schema schema,
- String tableName, TableConfig tableConfig, String segmentName, String sortedColumn,
- List<String> invertedIndexColumns, List<String> textIndexColumns, List<String> fstIndexColumns,
- List<String> noDictionaryColumns, List<String> varLengthDictionaryColumns, boolean nullHandlingEnabled) {
+ public RealtimeSegmentConverter(MutableSegmentImpl realtimeSegment, SegmentZKPropsConfig segmentZKMetadataConfig,
+ String outputPath, Schema schema, String tableName, TableConfig tableConfig, String segmentName,
+ String sortedColumn, List<String> invertedIndexColumns, List<String> textIndexColumns,
+ List<String> fstIndexColumns, List<String> noDictionaryColumns, List<String> varLengthDictionaryColumns,
+ boolean nullHandlingEnabled) {
_realtimeSegmentImpl = realtimeSegment;
+ _segmentZKMetadataConfig = segmentZKMetadataConfig;
Review Comment:
rename variable to match with new name of class
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -619,6 +621,12 @@ private void writeMetadata()
_dictionaryCreatorMap.containsKey(column), dictionaryElementSize);
}
+ SegmentZKPropsConfig segmentZKMetadataConfig = _config.getSegmentZKPropsConfig();
Review Comment:
rename variable
--
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] saurabhd336 commented on a diff in pull request #8502: Add zk metadata to segment's metadata.properties file
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #8502:
URL: https://github.com/apache/pinot/pull/8502#discussion_r847082067
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentZKMetadataConfig.java:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+// TODO Possibly redundant class. Use SegmentZKMetadata throughout?
Review Comment:
Will have to move SegmentZKMetadata out of pinot-commons to pinot-spi. I'll be checking if that could break something. If all is fine, will update the 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