Add default for EnginePlugin.getEngineFactory

[jmazanec15]

Description

Adds default implementation for getEngineFactory in EnginePlugin. The
default just returns Optional.empty(), allowing plugin developers to
implement this plugin without implementing this method.

Related issues: #1805

Issues Resolved

#2418

Check List

• 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.

[opensearch-ci-bot]

Can one of the admins verify this patch?

[dblock]

Add a test for this that creates an instance without overriding this method.

[owaiskazi19]

@jmazanec15 does other plugins will have to make any change on their end as well to incorporate this?

[opensearch-ci-bot]

❌   Gradle Check failure cc05bcc
Log 3202

Reports 3202

[jmazanec15]

@jmazanec15 does other plugins will have to make any change on their end as well to incorporate this?

No change needed. Adding default will just allow plugin implementors to skip implementing getEngineFactory as noop.

[opensearch-ci-bot]

❌   Gradle Check failure 9e10d0e
Log 3204

Reports 3204

[owaiskazi19]

@jmazanec15 can you create an issue to add tests for the other methods present in EnginePlugin.java? Thanks

[owaiskazi19]

start gradle check

[reta]

@jmazanec15 @owaiskazi19 there is a large number of tests under EngineConfigFactoryTests, it might be better to move the new one there as well.

[opensearch-ci-bot]

❌   Gradle Check failure 9e10d0e
Log 3207

Reports 3207

[jmazanec15]

@reta I can move the unit test to EngineConfigFactory, but what would the benefit be? It is not testing the functionality of the EngineConfigFactory, its more so testing the default of the interface works as expected.

Also, @reta @owaiskazi19 do you have any ideas on why "org.opensearch.cluster.allocation.ClusterRerouteIT.testDelayWithALargeAmountOfShards" is failing? It seems to be unrelated, but I cannot find reference elsewhere or similar failures on other PRs. Scanning the log, I see this:

  1> [2022-03-10T02:23:45,606][INFO ][o.o.c.m.MetadataIndexStateService] [node_t0] completed closing of indices [test12]
  1> [2022-03-10T02:23:48,598][INFO ][o.o.c.m.MetadataCreateIndexService] [node_t0] [test13] creating index, cause [api], templates [], shards [5]/[1]
  1> [2022-03-10T02:23:50,839][INFO ][o.o.c.r.a.AllocationService] [node_t0] Cluster health status changed from [RED] to [YELLOW] (reason: [shards started [[test12][1], [test12][3], [test12][2], [test12][0], [test12][4], [test13][1], [test13][3], [test13][4]]]).
  1> [2022-03-10T02:23:54,112][INFO ][o.o.c.m.MetadataIndexStateService] [node_t0] closing indices [test13/vo6FWb7_T-28zuI25nND3Q]
  1> [2022-03-10T02:23:54,285][INFO ][o.o.c.r.a.a.BalancedShardsAllocator] [node_t0] Cannot move any shard in the cluster as there is no node on which shards can be allocated. Skipping shard iteration
  1> [2022-03-10T02:23:54,799][INFO ][o.o.c.m.MetadataIndexStateService] [node_t0] completed closing of indices [test13]
  1> [2022-03-10T02:23:54,812][INFO ][o.o.c.r.a.a.BalancedShardsAllocator] [node_t0] Cannot move any shard in the cluster as there is no node on which shards can be allocated. Skipping shard iteration

...
  1> java.lang.IllegalStateException: transport not ready yet to handle incoming requests

but not much else. Also, I was not able to repro locally:

./gradlew ':server:internalClusterTest' --tests "org.opensearch.cluster.allocation.ClusterRerouteIT.testDelayWithALargeAmountOfShards" -Dtests.seed=FF9F1BDE6DF4DC66
Picked up JAVA_TOOL_OPTIONS: -Dlog4j2.formatMsgNoLookups=true

> Configure project :qa:os
Cannot add task 'destructiveDistroTest.docker' as a task with that name already exists.
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 7.3.3
  OS Info               : Mac OS X 10.16 (x86_64)
  JDK Version           : 14 (OpenJDK)
  JAVA_HOME             : /Users/jmazane/.sdkman/candidates/java/14.0.2-open
  Random Testing Seed   : FF9F1BDE6DF4DC66
  In FIPS 140 mode      : false
=======================================

> Task :server:internalClusterTest
Picked up JAVA_TOOL_OPTIONS: -Dlog4j2.formatMsgNoLookups=true
OpenJDK 64-Bit Server VM warning: Ignoring option --illegal-access=warn; support was removed in 17.0
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by org.opensearch.bootstrap.BootstrapForTesting (file:/Users/jmazane/workspace/Opensearch/mine/OpenSearch/test/framework/build/distributions/framework-2.0.0-SNAPSHOT.jar)
WARNING: Please consider reporting this to the maintainers of org.opensearch.bootstrap.BootstrapForTesting
WARNING: System::setSecurityManager will be removed in a future release
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by org.gradle.api.internal.tasks.testing.worker.TestWorker (file:/Users/jmazane/.gradle/wrapper/dists/gradle-7.3.3-all/4295vidhdd9hd3gbjyw1xqxpo/gradle-7.3.3/lib/plugins/gradle-testing-base-7.3.3.jar)
WARNING: Please consider reporting this to the maintainers of org.gradle.api.internal.tasks.testing.worker.TestWorker
WARNING: System::setSecurityManager will be removed in a future release

BUILD SUCCESSFUL in 3m 31s
44 actionable tasks: 1 executed, 43 up-to-date

[owaiskazi19]

@jmazanec15 it's a flaky test. We have an issue for it: #1561. Let me start the gradle check again.

[owaiskazi19]

start gradle check

[opensearch-ci-bot]

❌   Gradle Check failure 9e10d0e
Log 3209

Reports 3209

[reta]

@reta I can move the unit test to EngineConfigFactory, but what would the benefit be? It is not testing the functionality of the EngineConfigFactory, its more so testing the default of the interface works as expected.

If you look at this test case, EngineConfigFactoryTests, it basically exercises the large part of the EnginePlugin so we could add getEngineFactory tests as well, but this is just a suggestion (the benefit would be to keep EnginePlugin tests in a single place).

[jmazanec15]

@reta got it! Just moved the test

[opensearch-ci-bot]

✅   Gradle Check success 98f2f5e
Log 3265

Reports 3265

[nknize]

LGTM