HBASE-23093 : Avoid Optional Anti-Pattern where possible #673
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
virajjasani
force-pushed
the
HBASE-23093-master
branch
from
4440e6c to
215c726
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Please review @Apache9 @petersomogyi |
petersomogyi
requested changes
Oct 5, 2019
| @@ -400,18 +400,17 @@ private void sendToServer(ServerName serverName, ServerRequest serverReq, int tr | |||
| // based on the load of the region server and the region. | |||
| private void sendOrDelay(Map<ServerName, ServerRequest> actionsByServer, int tries) { | |||
| Optional<MetricsConnection> metrics = conn.getConnectionMetrics(); | |||
| Optional<ServerStatisticTracker> optStats = conn.getStatisticsTracker(); | |||
| if (!optStats.isPresent()) { | |||
| ServerStatisticTracker optStats = conn.getStatisticsTracker(); | |||
There was a problem hiding this comment.
The variable name can be stats now.
|
Added @Apache9 as a reviewer since he had some comments on the Jira. |
|
🎊 +1 overall
This message was automatically generated. |
petersomogyi
approved these changes
Oct 7, 2019
Apache9
requested changes
Oct 8, 2019
| @@ -281,7 +281,7 @@ void clearMasterStubCache(MasterService.Interface stub) { | |||
| masterStub.compareAndSet(stub, null); | |||
| } | |||
|
|
|||
| Optional<ServerStatisticTracker> getStatisticsTracker() { | |||
| ServerStatisticTracker getStatisticsTracker() { | |||
There was a problem hiding this comment.
Why this change? It could be null and it is a return value, not a parameter...
There was a problem hiding this comment.
This is just to simplify as we are putting this returned value directly as argument to some methods.
e.g.
https://github.com/apache/hbase/pull/673/files#diff-6705e0c1d8285fddc3d939ad3352b1a6R310
https://github.com/apache/hbase/pull/673/files#diff-e12d4030edb8885f06f86ee77cfb5b0cR383
There was a problem hiding this comment.
I think we should align the interface... It is a bit confusing that we return Optional for some methods, and then some other methods which do not return an Optional but could still return null...
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRegionLocatorHelper.java
Show resolved
Hide resolved
| @@ -415,13 +414,15 @@ static void incRegionCountMetrics(ScanMetrics scanMetrics) { | |||
| * increase the hedge read related metrics. | |||
| */ | |||
| private static <T> void connect(CompletableFuture<T> srcFuture, CompletableFuture<T> dstFuture, | |||
| Optional<MetricsConnection> metrics) { | |||
| MetricsConnection metrics) { | |||
There was a problem hiding this comment.
Here I do not think it is necessary to do this change? The upper layer will get an Optional, and then we convert it to an object or a null, by checking if the object is null, and here we check it again...
| @@ -435,23 +436,25 @@ static void incRegionCountMetrics(ScanMetrics scanMetrics) { | |||
|
|
|||
| private static <T> void sendRequestsToSecondaryReplicas( | |||
| Function<Integer, CompletableFuture<T>> requestReplica, RegionLocations locs, | |||
| CompletableFuture<T> future, Optional<MetricsConnection> metrics) { | |||
| CompletableFuture<T> future, MetricsConnection metrics) { | |||
| static void updateStats(Optional<ServerStatisticTracker> optStats, | ||
| Optional<MetricsConnection> optMetrics, ServerName serverName, MultiResponse resp) { | ||
| if (!optStats.isPresent() && !optMetrics.isPresent()) { | ||
| static void updateStats(ServerStatisticTracker serverStatisticTracker, |
| @@ -220,7 +220,8 @@ private static Result toResult(HBaseRpcController controller, MutateResponse res | |||
| public CompletableFuture<Result> get(Get get) { | |||
| return timelineConsistentRead(conn.getLocator(), tableName, get, get.getRow(), | |||
| RegionLocateType.CURRENT, replicaId -> get(get, replicaId), readRpcTimeoutNs, | |||
| conn.connConf.getPrimaryCallTimeoutNs(), retryTimer, conn.getConnectionMetrics()); | |||
| conn.connConf.getPrimaryCallTimeoutNs(), retryTimer, | |||
| conn.getConnectionMetrics().orElse(null)); | |||
There was a problem hiding this comment.
This is what I mean, we unwrap the Optional, and then check if it is null, this does not make sense...
...erver/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerWrapperImpl.java
Show resolved
Hide resolved
| @@ -351,11 +350,11 @@ public StoreScanner(ScanInfo scanInfo, ScanType scanType, | |||
|
|
|||
| // Used to instantiate a scanner for compaction in test | |||
| @VisibleForTesting | |||
| StoreScanner(ScanInfo scanInfo, OptionalInt maxVersions, ScanType scanType, | |||
| StoreScanner(ScanInfo scanInfo, Integer maxVersions, ScanType scanType, | |||
There was a problem hiding this comment.
Oh, what is the differece between an OptionalInt and an Integer? They are both wrapping types...
There was a problem hiding this comment.
True both are wrapping but OptionalInt is again wrapping Integer also.
Here we call this:
https://github.com/apache/hbase/pull/673/files#diff-ff286d61ea5468d0f8978181e4f3a896R946
https://github.com/apache/hbase/pull/673/files#diff-ff286d61ea5468d0f8978181e4f3a896R974
At least we can directly pass int value 2 or null rather than OptionalInt.of(2) and Optional.empty()
There was a problem hiding this comment.
Passing value 2 will introduce a Integer.valueOf(2) as you are passing an Integer, not a int. And Optional.empty is just a static object, which does not make any difference with null...
There was a problem hiding this comment.
sure, let me change this to as it was before
There was a problem hiding this comment.
And is it possible to just pass an int? I think not all the int values are valid for maxVersions, maybe we can use -1 to indicate null here?
| @@ -289,7 +289,7 @@ public boolean replicate(ReplicateContext replicateContext) { | |||
| } | |||
| continue outer; | |||
| } | |||
| if (!requiresReplication(tableDesc, entry)) { | |||
| if (!requiresReplication(tableDesc.orElse(null), entry)) { | |||
There was a problem hiding this comment.
OK, again, here we unwrap the optional by checking whether it is null and inside the method we checl it again...
Where does this Optional come from? If we could not change the root, let's keep it as is?
virajjasani
force-pushed
the
HBASE-23093-master
branch
from
c1118e5 to
e0a5f24
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
Will take a look this night... |
Apache9
requested changes
Oct 10, 2019
| @@ -8743,7 +8743,7 @@ public void registerChildren(ConfigurationManager manager) { | |||
| */ | |||
| @Override | |||
| public void deregisterChildren(ConfigurationManager manager) { | |||
| stores.values().forEach(configurationManager.get()::deregisterObserver); | |||
| stores.values().forEach(store -> configurationManager.deregisterObserver(store)); | |||
There was a problem hiding this comment.
Just use configurationManager::deregisterObserver?
| @@ -357,7 +357,7 @@ public boolean passesKeyRangeFilter(Scan scan) { | |||
| } | |||
| } | |||
| } | |||
| return this.firstKey; | |||
| return Optional.ofNullable(this.firstKey); | |||
There was a problem hiding this comment.
Not sure if this has been answered but I think this method will be called multiple times? And we always need to do a wrapping and a null check?
...erver/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerWrapperImpl.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
Show resolved
Hide resolved
Apache9
requested changes
Oct 10, 2019
| .of(((CombinedBlockCache.CombinedCacheStats) this.cacheStats.get()).getLruCacheStats()); | ||
| l2Stats = Optional.of(((CombinedBlockCache.CombinedCacheStats) this.cacheStats.get()) | ||
| .getBucketCacheStats()); | ||
| this.blockCache = this.regionServer.getBlockCache().isPresent() ? |
...erver/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerWrapperImpl.java
Show resolved
Hide resolved
| @@ -170,7 +170,8 @@ private void initBlockCache() { | |||
| * Initializes the mob file cache. | |||
| */ | |||
| private void initMobFileCache() { | |||
| this.mobFileCache = this.regionServer.getMobFileCache(); | |||
| this.mobFileCache = this.regionServer.getMobFileCache().isPresent() ? | |||
virajjasani
force-pushed
the
HBASE-23093-master
branch
from
e0a5f24 to
9ade583
Compare
|
💔 -1 overall
This message was automatically generated. |
|
TestBulkLoadReplication is passing locally |
asfgit
pushed a commit
that referenced
this pull request
Oct 11, 2019
Signed-off-by: Peter Somogyi <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Oct 11, 2019
Signed-off-by: Peter Somogyi <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Oct 11, 2019
Signed-off-by: Peter Somogyi <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
infraio
pushed a commit
to infraio/hbase
that referenced
this pull request
Aug 17, 2020
Signed-off-by: Peter Somogyi <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
symat
pushed a commit
to symat/hbase
that referenced
this pull request
Feb 17, 2021
Signed-off-by: Peter Somogyi <[email protected]> Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit 122b5b2) Change-Id: I0f995fdf1e4d6b19c39c8bf8f63066bf5843a81a
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.
Jira: https://issues.apache.org/jira/browse/HBASE-23093