Fix some problems reported by PVS-Studio #7738
Conversation
| @@ -274,7 +272,7 @@ private long getOffset(boolean earliest) throws InterruptedException | |||
| } | |||
| } | |||
|
|
|||
| return response != null ? filterAndDecode(response.messageSet(topic, partitionId), offset) : EMPTY_MSGS; | |||
| return filterAndDecode(response.messageSet(topic, partitionId), offset); | |||
There was a problem hiding this comment.
Is it impossible for response to be null at this point? I guess the idea is that if it's null, errorCode should be ErrorMapping.UnknownCode(), and therefore needNewLeader should be true, and so we should hit the continue; and keep looping.
The logic is a bit nontrivial to follow, so if that is accurate, a comment or Preconditions.checkState would be helpful.
There was a problem hiding this comment.
No, it's impossible. Open this class in IntelliJ - its data flow analysis identifies that the condition is always true as well.
Related to #7780.
| @@ -91,7 +91,7 @@ public CoordinatorBasicAuthenticatorMetadataStorageUpdater( | |||
| BasicAuthCommonCacheConfig commonCacheConfig, | |||
| @Smile ObjectMapper objectMapper, | |||
| BasicAuthenticatorCacheNotifier cacheNotifier, | |||
| ConfigManager configManager // ConfigManager creates the db table we need, set a dependency here | |||
| ConfigManager configManager // -V6022: ConfigManager creates the db table we need, set a dependency here | |||
There was a problem hiding this comment.
It means an unused parameter. I've extended the comment.
| @@ -52,7 +52,7 @@ public void kill(DataSegment segment) throws SegmentLoadingException | |||
| Map<String, Object> loadSpec = segment.getLoadSpec(); | |||
| final String bucket = MapUtils.getString(loadSpec, "bucket"); | |||
| final String indexPath = MapUtils.getString(loadSpec, "path"); | |||
| final String descriptorPath = indexPath.substring(0, indexPath.lastIndexOf('/')) + "/descriptor.json"; | |||
| final String descriptorPath = indexPath.substring(0, indexPath.lastIndexOf('/')) + "/descriptor.json"; //-V6009 | |||
There was a problem hiding this comment.
Refactored code to avoid the warning in the first place
There was a problem hiding this comment.
This inspection means "substring() argument can be negative".
| @@ -419,7 +419,7 @@ protected void mergeInsert(final int mergeAt, int insertAt, final float v, final | |||
| // use unused slot to shift array left or right and make space for the new bin to insert | |||
| if (insertAt < unusedIndex) { | |||
| shiftRight(insertAt, unusedIndex); | |||
| } else if (insertAt >= unusedIndex) { | |||
| } else { // insertAt >= unusedIndex | |||
There was a problem hiding this comment.
How about turning this into an assert instead of a comment (or just delete it, it's pretty obvious just from reading the code).
| @@ -241,6 +241,7 @@ public HttpResponseStatus getProxiedOverlordScalingResponseStatus() | |||
| ); | |||
| } | |||
|
|
|||
| @SuppressWarnings("unused") // It's unclear whether this call to readValue() has desirable side effects. | |||
There was a problem hiding this comment.
I don't think it has any meaningful side effects. response.getContent() is a string that's already been read, so it doesn't trigger reading a stream or anything like that. Should be fine to remove.
from #7738, this is a performance issue
from #7738, this is a performance issue
|
Thanks for review |
from apache#7738, this is a performance issue
* Fix some problems reported by PVS-Studio * Address comments
* Fix some problems reported by PVS-Studio * Address comments
Mostly cosmetics and unused code, no real bugs here.