Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade gradle version and remove remaining use of ImmutableOpenMap #814

Merged
merged 1 commit into from
May 10, 2023

Conversation

monusingh-1
Copy link
Collaborator

@monusingh-1 monusingh-1 commented Apr 27, 2023

Description

Changes made

  • upgraded gradle version from gradle-7.4.2 to gradle-7.6
  • Replaced existing uses of import java.util.* to relevant imports
  • Removed uses of ImmutableOpenMap and changed to Map
  • Added Null safety

Issues Resolved

Current builds started failing due to upstream changes in Opensearch

Opensearch removed the remaining uses of ImmutableOpenMap to Map.
the get() of ImmutableOpenMap return non-null empty set however the get() method of Map can return null. To avoid this we changed the uses of get() to getOrDefault() to handle null cases gracefully.

opensearch-project/OpenSearch@d984f50

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@monusingh-1 monusingh-1 changed the title Test Upgrade gradle version and remove remaining use of ImmutableOpenMap Apr 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2023

Codecov Report

Merging #814 (af0dd6b) into main (3b43e55) will increase coverage by 0.80%.
The diff coverage is 42.85%.

❗ Current head af0dd6b differs from pull request most recent head 43cde95. Consider uploading reports for the commit 43cde95 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff              @@
##               main     #814      +/-   ##
============================================
+ Coverage     72.59%   73.39%   +0.80%     
- Complexity     1003     1008       +5     
============================================
  Files           141      141              
  Lines          4652     4653       +1     
  Branches        520      526       +6     
============================================
+ Hits           3377     3415      +38     
+ Misses          943      915      -28     
+ Partials        332      323       -9     
Impacted Files Coverage Δ
...on/resume/TransportResumeIndexReplicationAction.kt 85.29% <0.00%> (-4.42%) ⬇️
...ication/seqno/RemoteClusterRetentionLeaseHelper.kt 76.66% <0.00%> (-3.34%) ⬇️
...rch/replication/task/index/IndexReplicationTask.kt 67.93% <38.88%> (-0.57%) ⬇️
...lication/metadata/TransportUpdateMetadataAction.kt 76.36% <100.00%> (ø)
...earch/replication/metadata/UpdateIndexBlockTask.kt 74.19% <100.00%> (ø)
.../replication/repository/RemoteClusterRepository.kt 73.13% <100.00%> (ø)

... and 6 files with indirect coverage changes

@monusingh-1 monusingh-1 marked this pull request as ready for review May 3, 2023 06:00
@monusingh-1 monusingh-1 requested a review from gbbafna as a code owner May 3, 2023 06:00
@@ -110,7 +109,7 @@ open class NoOpClient(testName :String) : NoOpNodeClient(testName) {
val indexToSettings = HashMap<String, Settings>()
indexToSettings[IndexReplicationTaskTests.followerIndex] = desiredSettingsBuilder.build()

val settingsMap = ImmutableOpenMap.builder<String, Settings>().putAll(indexToSettings).build()
val settingsMap = indexToSettings.toMutableMap()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be immutable map

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont want to be using Immutable as map as that implementation is being remove.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImmutableOpenMap is being removed. So you can use any other immutable implementation like Collections.unmodifiableMap

@@ -55,8 +54,7 @@ import org.opensearch.snapshots.RestoreInfo
import org.opensearch.test.OpenSearchTestCase
import org.opensearch.test.client.NoOpNodeClient
import java.lang.reflect.Field
import java.util.ArrayList
import java.util.HashMap
import java.util.*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wildcard import should not be used

soosinha
soosinha previously approved these changes May 8, 2023
val retentionLeaseHelper = RemoteClusterRetentionLeaseHelper(clusterService.clusterName.value(), remoteClient)
shards.forEach {
shards?.forEach {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know the changes in upstream that caused this?
At this point, we need these shards to exist as part of the cluster state. If it is not present(/null), let's log here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check similar cases in the PR.

Copy link
Member

@ankitkala ankitkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the added null safety checks, can you include details around why the changes were done?

Copy link
Member

@saikaranam-amazon saikaranam-amazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - let's keep an eye on the integ tests.

@saikaranam-amazon saikaranam-amazon merged commit 49e8781 into opensearch-project:main May 10, 2023
@opensearch-trigger-bot
Copy link

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-814-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 49e8781df5bf9d48525302bfc5e523c8c28c55ef
# Push it to GitHub
git push --set-upstream origin backport/backport-814-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-814-to-2.x.

monusingh-1 added a commit to monusingh-1/ccr-dev that referenced this pull request Jul 11, 2023
monusingh-1 added a commit that referenced this pull request Jul 11, 2023
* Open Upgrade gradle version and remove remaining use of ImmutableOpenMap (#814)

Signed-off-by: Monu Singh <[email protected]>

* Increment version to 2.9 and refactor

Signed-off-by: monusingh-1 <[email protected]>

* remove use of import java.util.*

Signed-off-by: monusingh-1 <[email protected]>

---------

Signed-off-by: Monu Singh <[email protected]>
Signed-off-by: monusingh-1 <[email protected]>
monusingh-1 added a commit to monusingh-1/ccr-dev that referenced this pull request Jul 24, 2023
monusingh-1 added a commit that referenced this pull request Jul 25, 2023
* Open Upgrade gradle version and remove remaining use of ImmutableOpenMap (#814)

Signed-off-by: Monu Singh <[email protected]>

* Move from common to core.common (#1087)

Keep up with core changes
opensearch-project/OpenSearch#8157
move from common to core.common

Change kotlin.version "1.8.21" as
Class 'org.opensearch.commons.utils.OpenForTesting' was compiled with "1.8.21" version of Kotlin. Ref

Set following to use 1.6 as 1.8.21 is not available yet. Checked that other OpenSearch plugins are also doing the same.
Set kotlinx-coroutines-core to 1.6.0
set kotlinx-coroutines-test to 1.6.0

Added else in when statements as from Kotlin 1.7 version onward exhaustive list is mandatory.
'when' expression must be exhaustive, add necessary 'else' branch

---------

Signed-off-by: monusingh-1 <[email protected]>

---------

Signed-off-by: Monu Singh <[email protected]>
Signed-off-by: monusingh-1 <[email protected]>
@monusingh-1 monusingh-1 deleted the test branch November 2, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants