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

Fix compile warnings in mantis-control-plane-server #417

Closed
wants to merge 4 commits into from
Closed

Fix compile warnings in mantis-control-plane-server #417

wants to merge 4 commits into from

Conversation

itsharis
Copy link

@itsharis itsharis commented Apr 12, 2023

Context

Progress towards: [#392]

  • Remove deprecated bindAndHandle
  • Remove deprecated Mockito.Matchers and replace with ArgumentMatchers
  • Remove deprecated ActorMaterializer and replace with Materializer
  • Remove deprecated startPeriodicTimer and replace with startTimerAtFixedRate
  • Fix serialization of Optional in unit tests
  • Replace DatatypeConverter with BaseEncoding

Checklist

  • ./gradlew build compiles code correctly
  • Added new tests where applicable
  • ./gradlew test passes all tests
  • Extended README or added javadocs where applicable

Note: There are more fixes for for this module, however but i am splitting them up so they are in a more consumable format

* Remove deprecated bindAndHandle
* Remove deprecated Mockito.Matchers and replace with ArgumentMatchers
* Remove deprecated ActorMaterializer and replace with Materializer
* Remove deprecated startPeriodicTimer and replace with startTimerAtFixedRate
* Fix serialization of Optional in unit tests
* Replace DatatypeConverter with BaseEncoding
import lombok.Value;
import org.apache.flink.shaded.guava30.com.google.common.io.BaseEncoding;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's a good idea to use the Flink-shaded version of guava. There is a mantis-shaded version.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks i will make this change.

@@ -67,16 +67,16 @@ public void testDeser4() throws IOException {
mwm.setCluster(Optional.of("test"));

final String out = objectMapper.writer(Jackson.DEFAULT_FILTER_PROVIDER).writeValueAsString(mwm);
assertTrue(out.contains("\"cluster\":{\"present\":true},"));
assertTrue(out.contains("\"cluster\":{\"empty\":false,\"present\":true}"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

where and why is this "empty" key introduced?

Copy link
Author

@itsharis itsharis Apr 17, 2023

Choose a reason for hiding this comment

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

Had to do some digging into this.

This is a difference with jakson on JDK 8 vs JDK 11 (JDK i am using on M1: https://adoptium.net/download/).
I thought setting the (inteliJ IDEA) File -> Project Structure- > Language level to 8 would be sufficient, however it dosent seem that way.
JDK 8 evaluates:
objectMapper.writeValueAsString(Optional.empty()) as {"present":false}
JDK 11 evaluates:
objectMapper.writeValueAsString(Optional.empty()) as {empty":true,"present":false}

Similarly:
objectMapper.writeValueAsString(Optional.of("test")) evaluates to {"empty":false,"present":true}
I will undo this change since sourceCompatibility is set Java 8

* Switch BaseEncoding.base16 to use mantisrx.shaded import
@itsharis itsharis had a problem deploying to Integrate Pull Request April 17, 2023 04:52 — with GitHub Actions Failure
@itsharis itsharis requested a review from Andyz26 April 17, 2023 04:54
@github-actions
Copy link

Test Results

  82 files   -   44    82 suites   - 44   2m 9s ⏱️ - 4m 7s
416 tests  - 119  406 ✔️  - 120  7 💤  - 1  3 +2 
416 runs   - 120  406 ✔️  - 121  7 💤  - 1  3 +2 

For more details on these failures, see this check.

Results for commit 9e754ac. ± Comparison against base commit 6fc313e.

This pull request removes 119 tests.
io.mantisrx.publish.AbstractSubscriptionTrackerTest ‑ testDefaultStreamKeyAsStreamName()
io.mantisrx.publish.AbstractSubscriptionTrackerTest ‑ testDiscardSubscriptionsBeyondMax()
io.mantisrx.publish.AbstractSubscriptionTrackerTest ‑ testMaxSubscriptionCountChange()
io.mantisrx.publish.AbstractSubscriptionTrackerTest ‑ testSubscriptionUpdate()
io.mantisrx.publish.AbstractSubscriptionTrackerTest ‑ testUpdateMultipleStreams()
io.mantisrx.publish.AbstractSubscriptionTrackerTest ‑ testUpdateMultipleStreamsWithUnionSubscriptions()
io.mantisrx.publish.DefaultSubscriptionTrackerTest ‑ testSubsNotRefreshOnNoRegisteredStreams()
io.mantisrx.publish.DefaultSubscriptionTrackerTest ‑ testSubscriptionsFetchFailureHandling()
io.mantisrx.publish.DefaultSubscriptionTrackerTest ‑ testSubscriptionsResolveToMajorityAmongWorkers()
io.mantisrx.publish.EventDrainerTest ‑ shouldDrainAndNoopForNonexistentSubscribers()
…

@Andyz26
Copy link
Collaborator

Andyz26 commented Apr 19, 2023

@itsharis please rebase to latest master and get a clean CI run.

@itsharis itsharis had a problem deploying to Integrate Pull Request April 20, 2023 02:19 — with GitHub Actions Failure
@itsharis itsharis had a problem deploying to Integrate Pull Request April 20, 2023 02:24 — with GitHub Actions Failure
@itsharis itsharis had a problem deploying to Integrate Pull Request April 20, 2023 02:37 — with GitHub Actions Failure
@itsharis
Copy link
Author

Not sure if this was the right approach:
i synced the repo via github web UI, and synced my forked branch with master.

If not, please advise what is the best way to do this.

@itsharis itsharis had a problem deploying to Integrate Pull Request April 24, 2023 14:11 — with GitHub Actions Failure
@itsharis itsharis closed this Oct 23, 2023
@itsharis itsharis deleted the haris/warnings-control-plane-server branch October 23, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants