You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tibrewalpratik17 (via GitHub)" <gi...@apache.org> on 2024/11/19 13:05:39 UTC
[PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
tibrewalpratik17 opened a new pull request, #14494:
URL: https://github.com/apache/pinot/pull/14494
Currently, extracting table type from segment name assumes that the realtime segment is of LLC naming convention. We have added `UploadedRealtimeSegmentName` naming convention for realtime segments as well. A small patch to allow table type extraction for `UploadedRealtimeSegmentName` segments as well.
This flow is used during reload and delete segment processes. Previously it was resulting in TableNotFoundException.
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #14494:
URL: https://github.com/apache/pinot/pull/14494#discussion_r1850516862
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,13 +426,14 @@ public SuccessResponse reloadSegment(
/**
* Helper method to find the existing table based on the given table name (with or without type suffix) and segment
* name.
- * TODO: Real-time table might also contain uploaded segments (not with LLC name), which is not handled here.
*/
private String getExistingTable(String tableName, String segmentName) {
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableName);
if (tableType == null) {
// Derive table type from segment name if the given table name doesn't have type suffix
- tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
Review Comment:
For LCCSegment too, it seems it is not guaranteed only for realtime table. There are no validations in `PinotSegmentUploadDownloadRestletResource` that LCCSegment should only be uploaded to realtime table and segment assignments for offline tables don't check segmentNames.
Additionally, there are places in code discouraging deriving table type from segmentName, I think we can either consolidate segmentName to table types in one place and/or enforce segmentNames to realtime table type:
```
// Fetch table name. Try to derive the table name from the parameter and then from segment metadata
String rawTableName;
if (StringUtils.isNotEmpty(tableName)) {
rawTableName = TableNameBuilder.extractRawTableName(tableName);
} else {
// TODO: remove this when we completely deprecate the table name from segment metadata
rawTableName = segmentMetadata.getTableName();
LOGGER.warn("Table name is not provided as request query parameter when uploading segment: {} for table: {}",
segmentName, rawTableName);
}
```
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana merged PR #14494:
URL: https://github.com/apache/pinot/pull/14494
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #14494:
URL: https://github.com/apache/pinot/pull/14494#discussion_r1850381910
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,13 +426,14 @@ public SuccessResponse reloadSegment(
/**
* Helper method to find the existing table based on the given table name (with or without type suffix) and segment
* name.
- * TODO: Real-time table might also contain uploaded segments (not with LLC name), which is not handled here.
*/
private String getExistingTable(String tableName, String segmentName) {
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableName);
if (tableType == null) {
// Derive table type from segment name if the given table name doesn't have type suffix
- tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
Review Comment:
@ankitsultana, the implementation is similar to LLCSegment.isLLCSegment and we have table name restriction to not use `__`.
```
Preconditions.checkArgument(!tableName.contains(TABLE_NAME_FORBIDDEN_SUBSTRING),
"'tableName' cannot contain double underscore ('__')");
```
It should be safe to assume this segment name will always mean a realtime table(based on javadocs).
Additionally, I think we can add a restriction to only allow `UploadedRealtimeSegmentName` for realtime tables to make these kind of segment -> table type derivation safer. [draft PR: #14508]
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14494:
URL: https://github.com/apache/pinot/pull/14494#discussion_r1849043166
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,13 +426,14 @@ public SuccessResponse reloadSegment(
/**
* Helper method to find the existing table based on the given table name (with or without type suffix) and segment
* name.
- * TODO: Real-time table might also contain uploaded segments (not with LLC name), which is not handled here.
*/
private String getExistingTable(String tableName, String segmentName) {
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableName);
if (tableType == null) {
// Derive table type from segment name if the given table name doesn't have type suffix
- tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
+ tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME
+ : (UploadedRealtimeSegmentName.isUploadedRealtimeSegmentName(segmentName) ? TableType.REALTIME
Review Comment:
Lgtm but ideally this should be a utility method somewhere. @Jackie-Jiang : I saw that you marked `SegmentName` as deprecated as part of removing HLC code. It only has 2 methods now both of which seem unused. Would it make sense to unmark it as Deprecated and keep this logic there?
Otherwise I feel like this is a common bug folks will run into
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14494:
URL: https://github.com/apache/pinot/pull/14494#discussion_r1850938515
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,13 +426,14 @@ public SuccessResponse reloadSegment(
/**
* Helper method to find the existing table based on the given table name (with or without type suffix) and segment
* name.
- * TODO: Real-time table might also contain uploaded segments (not with LLC name), which is not handled here.
*/
private String getExistingTable(String tableName, String segmentName) {
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableName);
if (tableType == null) {
// Derive table type from segment name if the given table name doesn't have type suffix
- tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
Review Comment:
Yeah but then should we start enforcing tableNameWithType or add this patch to fallback to extracting using segment name on a best effort basis? Worst case if segment naming is incorrect, we end up throwing TableNotFoundException which is okay i guess.
Long term maybe we should migrate all the APIs to a standard naming convention (using tableNameWithType always) but that seems to be a bigger alignment.
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #14494:
URL: https://github.com/apache/pinot/pull/14494#discussion_r1850921142
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,13 +426,14 @@ public SuccessResponse reloadSegment(
/**
* Helper method to find the existing table based on the given table name (with or without type suffix) and segment
* name.
- * TODO: Real-time table might also contain uploaded segments (not with LLC name), which is not handled here.
*/
private String getExistingTable(String tableName, String segmentName) {
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableName);
if (tableType == null) {
// Derive table type from segment name if the given table name doesn't have type suffix
- tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
Review Comment:
No, didn’t mean that. I see the current reloadSegment restful is already able to take tableNameWithType to extract table type. When table type can’t be retrieved that way, it falls back to derive type from segment name, which is not reliable even for LLC segment name anyway. Iiuc about those, then the changes in this PR 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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14494:
URL: https://github.com/apache/pinot/pull/14494#discussion_r1850915996
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,13 +426,14 @@ public SuccessResponse reloadSegment(
/**
* Helper method to find the existing table based on the given table name (with or without type suffix) and segment
* name.
- * TODO: Real-time table might also contain uploaded segments (not with LLC name), which is not handled here.
*/
private String getExistingTable(String tableName, String segmentName) {
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableName);
if (tableType == null) {
// Derive table type from segment name if the given table name doesn't have type suffix
- tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
Review Comment:
@klsince do you recommend writing another API and deprecating this one (we cannot update this one for backward compatibility imo)? Or is it okay to use `isUploadedRealtimeSegmentName`?
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14494:
URL: https://github.com/apache/pinot/pull/14494#issuecomment-2485760828
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14494?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`ef0f5ed`)](https://app.codecov.io/gh/apache/pinot/commit/ef0f5edb59fcf11d587a760f1b7735e15804686e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1358 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (ef0f5ed). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (ef0f5ed) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|0|
>|unittests|5|0|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14494 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 3 -2433
Lines 133233 6 -133227
Branches 20636 0 -20636
=============================================
- Hits 82274 0 -82274
+ Misses 44911 6 -44905
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-27.73%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/14494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14494?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
----
🚨 Try these New Features:
- [Flaky Tests Detection](https://docs.codecov.com/docs/test-result-ingestion-beta) - Detect and resolve failed and flaky tests
- [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis) - Avoid shipping oversized bundles
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on PR #14494:
URL: https://github.com/apache/pinot/pull/14494#issuecomment-2486753814
Tagging @klsince and @rohityadav1993 as they have relevant context on naming here.
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #14494:
URL: https://github.com/apache/pinot/pull/14494#discussion_r1850921142
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,13 +426,14 @@ public SuccessResponse reloadSegment(
/**
* Helper method to find the existing table based on the given table name (with or without type suffix) and segment
* name.
- * TODO: Real-time table might also contain uploaded segments (not with LLC name), which is not handled here.
*/
private String getExistingTable(String tableName, String segmentName) {
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableName);
if (tableType == null) {
// Derive table type from segment name if the given table name doesn't have type suffix
- tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
Review Comment:
No, didn’t mean that. I see the current reloadSegment restful is already able to take tableNameWithType to extract table type. When table type can’t be retrieved that way, it falls back to derive type from segment name, which is not reliable even for LLC segment name anyway.
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14494:
URL: https://github.com/apache/pinot/pull/14494#discussion_r1849051190
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,13 +426,14 @@ public SuccessResponse reloadSegment(
/**
* Helper method to find the existing table based on the given table name (with or without type suffix) and segment
* name.
- * TODO: Real-time table might also contain uploaded segments (not with LLC name), which is not handled here.
*/
private String getExistingTable(String tableName, String segmentName) {
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableName);
if (tableType == null) {
// Derive table type from segment name if the given table name doesn't have type suffix
- tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
Review Comment:
Is it safe to assume that `UploadedRealtimeSegmentName.isUploadedRealtimeSegmentName(segmentName)` will not return true for an offline table? I saw that it simply checks for the count of double underscores and returns true if there are 4 of them.
```
/**
* Checks if the segment name is of the format: {prefix}__{tableName}__{partitionId}__{creationTime}__{suffix}
* @param segmentName
* @return boolean true if the segment name is of the format: {prefix}__{tableName}__{partitionId}__{creationTime}
* __{suffix}
*/
public static boolean isUploadedRealtimeSegmentName(String segmentName) {
int numSeparators = 0;
int index = 0;
while ((index = segmentName.indexOf(SEPARATOR, index)) != -1) {
numSeparators++;
index += 2; // SEPARATOR.length()
}
return numSeparators == 4;
}
```
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #14494:
URL: https://github.com/apache/pinot/pull/14494#discussion_r1850381910
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,13 +426,14 @@ public SuccessResponse reloadSegment(
/**
* Helper method to find the existing table based on the given table name (with or without type suffix) and segment
* name.
- * TODO: Real-time table might also contain uploaded segments (not with LLC name), which is not handled here.
*/
private String getExistingTable(String tableName, String segmentName) {
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableName);
if (tableType == null) {
// Derive table type from segment name if the given table name doesn't have type suffix
- tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
Review Comment:
@ankitsultana, the implementation is similar to LLCSegment.isLLCSegment and we have table name restriction to not use `__`.
```
Preconditions.checkArgument(!tableName.contains(TABLE_NAME_FORBIDDEN_SUBSTRING),
"'tableName' cannot contain double underscore ('__')");
```
It should be safe to assume this segment name will always mean a realtime table(based on javadocs).
Additionally, I think we can add a restriction to only allow `UploadedRealtimeSegmentName` for realtime tables to make these kind of segment -> table type derivation safer.
--
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
Re: [PR] Allow reload for UploadedRealtimeSegmentName segments [pinot]
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #14494:
URL: https://github.com/apache/pinot/pull/14494#discussion_r1850747333
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,13 +426,14 @@ public SuccessResponse reloadSegment(
/**
* Helper method to find the existing table based on the given table name (with or without type suffix) and segment
* name.
- * TODO: Real-time table might also contain uploaded segments (not with LLC name), which is not handled here.
*/
private String getExistingTable(String tableName, String segmentName) {
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableName);
if (tableType == null) {
// Derive table type from segment name if the given table name doesn't have type suffix
- tableType = LLCSegmentName.isLLCSegment(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
Review Comment:
yeah, isLLCSegment and isUploadedRealtimeSegmentName are doing similar checks. And using segment name to derive table type is just best effort and not very reliable as far as I can tell. It's better to call the reloadSegment API with `tableNameWithType` as the type can be extracted this table name more reliably.
--
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