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

Unable to use Systemd module with tar distribution #3755

Merged
merged 3 commits into from
Jul 14, 2022

Conversation

Ripolin
Copy link
Contributor

@Ripolin Ripolin commented Jul 1, 2022

Description

PR to remove distribution check when using a Systemd notify service

Issues Resolved

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.

@Ripolin Ripolin requested review from a team and reta as code owners July 1, 2022 13:27
@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2022

Gradle Check (Jenkins) Run Completed with:

@dblock
Copy link
Member

dblock commented Jul 5, 2022

@reta You've dug deeper into these gradle plugins. Any concerns?

@owaiskazi19
Copy link
Member

owaiskazi19 commented Jul 5, 2022

Execution failed for task ':modules:systemd:spotlessJavaCheck'.
> The following files had format violations:
      src/main/java/org/opensearch/systemd/SystemdPlugin.java
          @@ -35,7 +35,6 @@
           import·org.apache.logging.log4j.LogManager;
           import·org.apache.logging.log4j.Logger;
           import·org.apache.lucene.util.SetOnce;
          -import·org.opensearch.Build;
           import·org.opensearch.client.Client;
           import·org.opensearch.cluster.metadata.IndexNameExpressionResolver;
           import·org.opensearch.cluster.service.ClusterService;
      src/test/java/org/opensearch/systemd/SystemdPluginTests.java
          @@ -32,7 +32,6 @@
           
           package·org.opensearch.systemd;
           
          -import·org.opensearch.Build;
           import·org.opensearch.common.CheckedConsumer;
           import·org.opensearch.common.unit.TimeValue;
           import·org.opensearch.test.OpenSearchTestCase;
          @@ -66,6 +65,7 @@
           ········when(threadPool.scheduleWithFixedDelay(any(Runnable.class),·eq(TimeValue.timeValueSeconds(15)),·eq(ThreadPool.Names.SAME)))
           ············.thenReturn(extender);
           ····}
          +
           ····public·void·testIsImplicitlyNotEnabled()·{
           ········final·SystemdPlugin·plugin·=·new·SystemdPlugin(null);
           ········plugin.createComponents(null,·null,·threadPool,·null,·null,·null,·null,·null,·null,·null,·null);

@Ripolin can you run /gradlew :modules:systemd:spotlessApply to fix these violations and push the change.

@reta
Copy link
Collaborator

reta commented Jul 5, 2022

@reta You've dug deeper into these gradle plugins. Any concerns?

@dblock interesting, this is not a Gradle but Opensearch plugin, which is tight to distribution type BUT it is only included into packaged distribution [1] (@Ripolin you may need to update the build so the module is included into another distribution types).

[1] https://github.com/opensearch-project/OpenSearch/blob/main/distribution/build.gradle#L219

@Ripolin
Copy link
Contributor Author

Ripolin commented Jul 6, 2022

@owaiskazi19 Unused imports removed

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

Gradle Check (Jenkins) Run Completed with:

@Ripolin
Copy link
Contributor Author

Ripolin commented Jul 9, 2022

@dblock interesting, this is not a Gradle but Opensearch plugin, which is tight to distribution type BUT it is only included into packaged distribution [1] (@Ripolin you may need to update the build so the module is included into another distribution types).

[1] https://github.com/opensearch-project/OpenSearch/blob/main/distribution/build.gradle#L219

@dblock @reta Is it to add systemd module in tar package ? Can it create issues if use on windows systems ?

@reta
Copy link
Collaborator

reta commented Jul 11, 2022

@dblock interesting, this is not a Gradle but Opensearch plugin, which is tight to distribution type BUT it is only included into packaged distribution [1] (@Ripolin you may need to update the build so the module is included into another distribution types).
[1] https://github.com/opensearch-project/OpenSearch/blob/main/distribution/build.gradle#L219

@dblock @reta Is it to add systemd module in tar package ? Can it create issues if use on windows systems ?

@Ripolin yes, the change you did for a plugin (module) fixes only the checks but not the distribution part - the module won't be included in archive distributions (it is only applied to :distribution:packages:* targets, not archives). The relevant archives are platform specific (check please distribution/archives) so we could only add the module to Linux/Freebsd tar's (not Windows or Mac OS).

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@Ripolin
Copy link
Contributor Author

Ripolin commented Jul 13, 2022

@reta Commit ff1cd1a09042588f81fd0d5a1d81f5e2a63597e3 add systemd module for all linux and freebsd distributions

@reta
Copy link
Collaborator

reta commented Jul 13, 2022

@Ripolin the builds are failing, could you please rebase against lastest main? thank you

Ripolin added 3 commits July 13, 2022 17:50
Signed-off-by: Florent David <[email protected]>
Signed-off-by: Florent David <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

Codecov Report

Merging #3755 (ddf47d7) into main (2763e80) will increase coverage by 0.12%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##               main    #3755      +/-   ##
============================================
+ Coverage     70.47%   70.60%   +0.12%     
- Complexity    56617    56650      +33     
============================================
  Files          4557     4557              
  Lines        272683   272676       -7     
  Branches      40038    40034       -4     
============================================
+ Hits         192163   192512     +349     
+ Misses        64254    63882     -372     
- Partials      16266    16282      +16     
Impacted Files Coverage Δ
...ain/java/org/opensearch/systemd/SystemdPlugin.java 74.41% <50.00%> (+0.41%) ⬆️
...cluster/coordination/PublishClusterStateStats.java 33.33% <0.00%> (-37.51%) ⬇️
...java/org/opensearch/threadpool/ThreadPoolInfo.java 56.25% <0.00%> (-37.50%) ⬇️
...search/search/aggregations/pipeline/EwmaModel.java 24.44% <0.00%> (-33.34%) ⬇️
...nsearch/index/shard/IndexShardClosedException.java 66.66% <0.00%> (-33.34%) ⬇️
...va/org/opensearch/monitor/process/ProcessInfo.java 68.00% <0.00%> (-24.00%) ⬇️
...pensearch/index/shard/SearchOperationListener.java 72.34% <0.00%> (-23.41%) ⬇️
...ter/coordination/CoordinationStateTestCluster.java 73.78% <0.00%> (-23.18%) ⬇️
.../search/aggregations/pipeline/HoltLinearModel.java 20.33% <0.00%> (-22.04%) ⬇️
...luster/routing/allocation/RoutingExplanations.java 41.37% <0.00%> (-20.69%) ⬇️
... and 485 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2763e80...ddf47d7. Read the comment docs.

@dblock dblock merged commit 2858eb1 into opensearch-project:main Jul 14, 2022
@dblock dblock added the backport 2.x Backport to 2.x branch label Jul 14, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 14, 2022
* Fix #3666

Signed-off-by: Florent David <[email protected]>

* Fix linter issues

Signed-off-by: Florent David <[email protected]>

* Add systemd module in linux and freebsd archives

Signed-off-by: Florent David <[email protected]>
(cherry picked from commit 2858eb1)
reta pushed a commit that referenced this pull request Jul 14, 2022
* Fix #3666

Signed-off-by: Florent David <[email protected]>

* Fix linter issues

Signed-off-by: Florent David <[email protected]>

* Add systemd module in linux and freebsd archives

Signed-off-by: Florent David <[email protected]>
(cherry picked from commit 2858eb1)

Co-authored-by: Ripolin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants