Uh oh!
There was an error while loading. Please reload this page.
HBASE-25249 Adding StoreContext - #2800
Merged
Merged
Conversation
Apache-HBase
commented
Dec 21, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 21, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 21, 2020
💔 -1 overall
This message was automatically generated. |
apurtell
requested changes
Dec 21, 2020
Uh oh!
There was an error while loading. Please reload this page.
| return storeContext.getCacheConf(); | ||
| } | ||
| public static final long FIXED_OVERHEAD = ClassSize.estimateBase(HStore.class, false); |
Contributor
There was a problem hiding this comment.
Does DEEP_OVERHEAD and heapSize() need to change? Does TestHeapSize still pass?
ContributorAuthor
There was a problem hiding this comment.
TestHeapSize still passed when I checked it locally.
from what I understood from the FIXED_OVERHEAD were being calculated by ClassSize.estimateBase, it will automatically calculate the our newly added fields and reference included the change of HStoreContext storeContext.
So for the DEEP_OVERHEAD that takes new calculated FIXED_OVERHEAD to come up the heap size, it should be done already. Or did I miss something here?
ContributorAuthor
There was a problem hiding this comment.
this has been change as well, can you have another look ?
Uh oh!
There was an error while loading. Please reload this page.
taklwu
commented
Dec 22, 2020
ContributorAuthor
@apurtell requested changes updated. I shouldn't rebase and squash for the updated changes, will be providing a new diff on top of the first/original change next time |
Apache-HBase
commented
Dec 22, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 23, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 23, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 23, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 23, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 23, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 23, 2020
🎊 +1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Dec 23, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 23, 2020
🎊 +1 overall
This message was automatically generated. |
Adding HStoreContext which contains the metadata about the HStore. This meta data can be used across the HFileWriter/Readers and other HStore consumers without the need of passing around the complete store and exposing its internals.
- rename HStoreContext to StoreContext - rewrite the getChecksumType - fix one more style issues
z-york
reviewed
Dec 23, 2020
z-york
left a comment
Contributor
There was a problem hiding this comment.
Non-blocking minor comments
| private AtomicLong compactedCellsSize = new AtomicLong(); | ||
| private AtomicLong majorCompactedCellsSize = new AtomicLong(); | ||
| private StoreContext storeContext; |
Contributor
There was a problem hiding this comment.
Should this be final? I wouldn't expect you would need to change StoreContext (the reference) after it has been initialized
ContributorAuthor
There was a problem hiding this comment.
you're right, it could be final, will do it in next commit
| .withCompressTags(family.isCompressTags()) | ||
| .withChecksumType(checksumType) | ||
| .withBytesPerCheckSum(bytesPerChecksum) | ||
| .withCompressTags(getColumnFamilyDescriptor().isCompressTags()) |
Contributor
There was a problem hiding this comment.
Does it simplify things to be able to pass StoreContext directly to the builders?
ContributorAuthor
There was a problem hiding this comment.
I'd like to keep the current way that passing required fields to the creation of HFileContext. Mainly HFileContext is also a member of the StoreContext which if we pass into StoreContext to the creation of HFileContext, it will be a loop and is very strange.
Apache-HBase
commented
Dec 23, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 23, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 24, 2020
🎊 +1 overall
This message was automatically generated. |
saintstack
left a comment
Contributor
There was a problem hiding this comment.
On StoreContext vs StoreInfo, we have HFileContext so Context would work too. I'm not particular. There is info in the below comments that might help you if you considering name change.
There are a few odd things in the below. See what you think. Thanks.
| public void setCompression(Compression.Algorithm compressAlgo) { | ||
| this.compressAlgo = compressAlgo; | ||
| } | ||
Contributor
There was a problem hiding this comment.
Whats going on here?
HFileContext is supposed to be ' Read-only HFile Context Information' but here we are adding a setter. I see there are one or two setters but we also have HFileContextBuilder. Shouldn't we be going via the Builder making HFileContexts? And why is this method not added on the Builder? (Can it be added non-public to encourage users to go via the Builder)?
ContributorAuthor
There was a problem hiding this comment.
fixed, and reverted back to builder pattern
Contributor
There was a problem hiding this comment.
You haven't uploaded new PR so will leave this as unresovled for now.
ContributorAuthor
There was a problem hiding this comment.
oops, I missed this change. and now it should have updated/removed.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| return this.storeContext.getFamily(); | ||
| } | ||
| public Encryption.Context getEncryptionContext() { |
Contributor
There was a problem hiding this comment.
Has to be public? Has to be on HStore? Can it be on StoreContext? Is there a getStoreContext method on HStore?
ContributorAuthor
There was a problem hiding this comment.
getEncryptionContext was used by HMobStore such we set it to public , it could be package-private.
getStoreContext was planned to add in a followup PR when used, tho you're right that if we have a getStoreContext then this getEncryptionContext could be removed
Contributor
There was a problem hiding this comment.
Package private sounds good if only used locally.
| fileContext.setIncludesMvcc(includeMVCCReadpoint); | ||
| fileContext.setIncludesTags(includesTag); | ||
| fileContext.setCompression(compression); | ||
| fileContext.setFileCreateTime(EnvironmentEdgeManager.currentTime()); |
Contributor
There was a problem hiding this comment.
Why would we do this here and not as methods on the builder?
ContributorAuthor
There was a problem hiding this comment.
it was trying to reuse the defaultFileContext built with initializeStoreContext, but after rethinking on your comment, I'm going to remove the defaultFileContext and go back with the original builder pattern.
Contributor
There was a problem hiding this comment.
That'd be better. If a builder, lets use it everywhere (can add a constructor on builder that allows a bit of shortcutting auto-filling most fields....)
| /** | ||
| * This carries the information on some of the meta data about the HStore. This | ||
| * meta data can be used across the HFileWriter/Readers and other HStore consumers without the | ||
| * need of passing around the complete store. |
Contributor
There was a problem hiding this comment.
I would like to know here in class comment if this is read-only/immutable. I think it should be.
Contributor
There was a problem hiding this comment.
Also, just a comment... the HFileContext has this for a comment...
- Read-only HFile Context Information. Meta data that is used by HFileWriter/Readers and by
- HFileBlocks. Create one using the {@link HFileContextBuilder} (See HFileInfo and the HFile
- Trailer class).
so it is for readers and writers.... So, StoreContext is probably fine but if you are wondering... here is incidences of Info.java vs Context.java.... If it helps.
kalashnikov:hbase.apache.git stack$ find src/main/java -name *Info.java
find: src/main/java: No such file or directory
kalashnikov:hbase.apache.git stack$ find hbase-*/src/main/java -name *Info.java
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupTableInfo.java
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
hbase-client/src/main/java/org/apache/hadoop/hbase/security/SecurityInfo.java
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MutableRegionInfo.java
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionInfo.java
hbase-common/src/main/java/org/apache/hadoop/hbase/util/VersionInfo.java
hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java
hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/DrillDownInfo.java
hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/field/FieldInfo.java
hbase-metrics-api/src/main/java/org/apache/hadoop/hbase/metrics/MetricRegistryInfo.java
hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationQueueInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/HbckTableInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/HbckRegionInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockWithScanInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/namespace/NamespaceTableAndRegionInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/master/webapp/RegionReplicaInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/CallQueueInfo.java
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/generated/THRegionInfo.java
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift/generated/TRegionInfo.java
kalashnikov:hbase.apache.git stack$ find hbase-*/src/main/java -name *Context.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/Context.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/TagCompressionContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/RowPrefixFixedLengthBloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/RowBloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/RowColBloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/ReaderContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/ObserverContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/CompressionContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScannerContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/CompactionContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NoLimitScannerContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlushContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCallContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcSchedulerContext.java
ContributorAuthor
There was a problem hiding this comment.
thanks for sharing a list of *Info and *Context, I browsed few of them and seems Context should be still okie. so let's keep it with StoreContext and made this change simple, and I add the immutable keyword in the block of the class comment.
Apache-HBase
commented
Jan 6, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 6, 2021
🎊 +1 overall
This message was automatically generated. |
saintstack
requested changes
Jan 6, 2021
saintstack
left a comment
Contributor
There was a problem hiding this comment.
Ok. Seems like you are working on the changes so I'll mark this a 'Request Changes' till new PR. Thanks.
| public void setCompression(Compression.Algorithm compressAlgo) { | ||
| this.compressAlgo = compressAlgo; | ||
| } | ||
Contributor
There was a problem hiding this comment.
You haven't uploaded new PR so will leave this as unresovled for now.
…ation in constructor of HStore
Apache-HBase
commented
Jan 6, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 6, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 6, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 6, 2021
🎊 +1 overall
This message was automatically generated. |
| policyName = this.conf.get(BLOCK_STORAGE_POLICY_KEY, DEFAULT_BLOCK_STORAGE_POLICY); | ||
| } | ||
| this.fs.setStoragePolicy(family.getNameAsString(), policyName.trim()); | ||
| getRegionFileSystem().setStoragePolicy(getColumnFamilyName(), policyName.trim()); |
Contributor
There was a problem hiding this comment.
You were going to change these back?
ContributorAuthor
There was a problem hiding this comment.
changed back to use family and region.getRegionFileSystem() directly.
| } | ||
| this.storeEngine = createStoreEngine(this, this.conf, this.comparator); | ||
| this.storeEngine = createStoreEngine(this, this.conf, getComparator()); |
ContributorAuthor
There was a problem hiding this comment.
changed to use region.getCellComparator().
| int PRIORITY_USER = 1; | ||
| int NO_PRIORITY = Integer.MIN_VALUE; | ||
| int getBlockSize(); |
Contributor
There was a problem hiding this comment.
Is this necessary? Its available on the ColumnFamilyDescriptor. Do we need to add new method on Store?
Was this change always here or did it just show up in recent amendments to PRs (I don't remember seeing it before but probably just me).
Thanks.
ContributorAuthor
There was a problem hiding this comment.
this was new change in the latest commit of this PR, mainly it's used for HMobStore when calling createWriterInTmp. but let me change it using getStoreContext().getBlockSize() to avoid adding a new interface.
Contributor
There was a problem hiding this comment.
Yes. Please. Then I'll +1 this nice PR. Thanks.
saintstack
commented
Jan 6, 2021
Contributor
Just a few nits.... |
saintstack
approved these changes
Jan 6, 2021
Apache-HBase
commented
Jan 6, 2021
🎊 +1 overall
This message was automatically generated. |
z-york
approved these changes
Jan 6, 2021
Apache-HBase
commented
Jan 6, 2021
🎊 +1 overall
This message was automatically generated. |
taklwu
commented
Jan 6, 2021
Comment on lines
+115
to
+118
| @Override | ||
| public long heapSize() { | ||
| return FIXED_OVERHEAD; | ||
| } |
ContributorAuthor
There was a problem hiding this comment.
@apurtell ping again on the heapSize() and implements HeapSize for StoreContext class, could you have another look? I should have fixed it, but will wait for your approval/resolve in 1 day or 2 day before merging it.
ContributorAuthor
There was a problem hiding this comment.
@apurtell sorry for pinging again, I will merge this evening if I don't hear from you
Apache-HBase
commented
Jan 6, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 7, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 7, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 7, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 8, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 8, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 8, 2021
🎊 +1 overall
This message was automatically generated. |
This was referenced Dec 27, 2021
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Adding StoreContext which contains the metadata about the HStore. This
meta data can be used across the HFileWriter/Readers and other HStore
consumers without the need of passing around the complete store and
exposing its internals.