Cleaning up getTableName() for segment metadata #4289
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
snleee
force-pushed
the
segment-metadata-tablename-cleanup
branch
3 times, most recently
from
a64b460
to
33c9fab
Compare
Removing unnecessary calls to segmentMetadata.getTableName()
snleee
force-pushed
the
segment-metadata-tablename-cleanup
branch
from
33c9fab
to
25628d1
Compare
Codecov Report
@@ Coverage Diff @@
## master #4289 +/- ##
============================================
- Coverage 67.29% 67.09% -0.21%
Complexity 20 20
============================================
Files 1040 1041 +1
Lines 51662 51713 +51
Branches 7238 7247 +9
============================================
- Hits 34767 34696 -71
- Misses 14507 14651 +144
+ Partials 2388 2366 -22
Continue to review full report at Codecov.
|
|
Can you add more descriptions? The only time when you call SegmentMetadata.getTableName() is during segment push? |
I have updated the commit message.
|
mcvsubbu
reviewed
Jun 10, 2019
| @@ -303,35 +303,37 @@ private SuccessResponse uploadSegment(FormDataMultiPart multiPart, boolean enabl | |||
| throw new UnsupportedOperationException("Unsupported upload type: " + uploadType); | |||
| } | |||
|
|
|||
| String rawTableName = segmentMetadata.getTableName(); | |||
There was a problem hiding this comment.
All other temporary variables in the same code block is not using final. Do you think that we still need final?
| } | ||
|
|
||
| String clientAddress = InetAddress.getByName(request.getRemoteAddr()).getHostName(); | ||
| String segmentName = segmentMetadata.getName(); | ||
| String offlineTableName = TableNameBuilder.OFFLINE.tableNameWithType(segmentMetadata.getTableName()); | ||
| String offlineTableName = TableNameBuilder.OFFLINE.tableNameWithType(rawTableName); |
mcvsubbu
approved these changes
Jun 10, 2019
As we discussed offline, I will deprecate |
snleee
pushed a commit
to snleee/pinot
that referenced
this pull request
Jun 14, 2019
This is a follow up work for apache#4288 and apache#4289 1. Added deprecated annotation for segment metadata & segment zk metadata. 2. Remove all getTableName & setTableName usages from testing code - There is no usage of getTableName() for segment zk metadata except the case where we fill segment metadata from segment zk metadata for realtime segment creation. - For segment metadata, the only usage is during the segment upload - We still write segment name to both segment zk metadata and segment metadata for rollback support.
snleee
pushed a commit
to snleee/pinot
that referenced
this pull request
Jun 14, 2019
This is a follow up work for apache#4288 and apache#4289 1. Added deprecated annotation for segment zk metadata. 2. Remove all getTableName & setTableName usages from testing code - There is no usage of getTableName() for segment zk metadata except the case where we fill segment metadata from segment zk metadata for realtime segment creation. - For segment metadata, the only usage is during the segment upload - We still write segment name to both segment zk metadata and segment metadata for rollback support.
snleee
pushed a commit
that referenced
this pull request
Jun 18, 2019
* Add deprecated annotation for segment zk metadata This is a follow up work for #4288 and #4289 1. Added deprecated annotation for segment zk metadata. 2. Remove all getTableName & setTableName usages from testing code - There is no usage of getTableName() for segment zk metadata except the case where we fill segment metadata from segment zk metadata for realtime segment creation. - For segment metadata, the only usage is during the segment upload - We still write segment name to both segment zk metadata and segment metadata for rollback support. * Addressing comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removing unnecessary calls to segmentMetadata.getTableName()
After this change, only 3 places will call
segmentMetadata.getTableName():ZKMetadataUtil.updateSegmentMetadata()- updating segment zk metadata from segment metadata during segment uploadColumnarToStarTreeConverterprovides table name to generator config from segment metadata.