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

nixos/druid: init module and package #219942

Merged
merged 4 commits into from
Jul 28, 2024
Merged

Conversation

vsharathchandra
Copy link

@vsharathchandra vsharathchandra commented Mar 7, 2023

Description of changes
  • init apache druid at 29.0.0
  • init druid module
  • init druid nixos tests

Druid is a high performance, real-time analytics database that delivers sub-second queries on streaming and batch data at scale and under load.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 7, 2023
@illustris
Copy link
Contributor

illustris commented Mar 10, 2023

editorconfig:

nix run nixpkgs#nixpkgs-fmt nixos/tests/druid/with_hdfs.nix

nixos manual:

All options need to be updated to markdown. For example,

diff --git a/nixos/modules/services/cluster/druid/default.nix b/nixos/modules/services/cluster/druid/default.nix
index 62c100ad8a4..a756c6edd07 100644
--- a/nixos/modules/services/cluster/druid/default.nix
+++ b/nixos/modules/services/cluster/druid/default.nix
@@ -4,10 +4,10 @@ let
   cfg = config.services.druid;

   druidServiceOption = serviceName: {
-    enable = mkEnableOption serviceName;
+    enable = mkEnableOption (mdDoc serviceName);
     restartIfChanged = mkOption {
       type = types.bool;
-      description = ''
+      description = mdDoc ''
         Automatically restart the service on config change.
         This can be set to false to defer restarts on clusters running critical applications.
         Please consider the security implications of inadvertently running an older version,
@@ -18,26 +18,26 @@ let
     config = mkOption {
       default = { };
       type = types.attrsOf types.anything;
-      description = "(key=value) Configuration to be written to runtime.properties of the druid ${serviceName}";
+      description = mdDoc "(key=value) Configuration to be written to runtime.properties of the druid ${serviceName}";
     };
-    jdk = mkPackageOption pkgs "JDK" {
+    jdk = mkPackageOptionMD pkgs "JDK" {
       default = [ "jdk8_headless" ];
     };

restricted eval:

replace fetchTarball with fetchurl

I have a few more suggestions, but can you push these changes to see if CI passes first?

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 17, 2023
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/tests/all-tests.nix Outdated Show resolved Hide resolved
nixos/tests/druid/with_hdfs.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 13, 2023
@h7x4 h7x4 added 8.has: module (new) This PR adds a module in `nixos/` 8.has: tests This PR has tests labels Oct 22, 2023
@vsharathchandra vsharathchandra marked this pull request as draft January 21, 2024 11:36
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/cluster/druid/default.nix Outdated Show resolved Hide resolved
@vsharathchandra vsharathchandra marked this pull request as ready for review March 6, 2024 17:02
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3589

nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 20, 2024
Copy link
Contributor

@illustris illustris left a comment

Choose a reason for hiding this comment

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

Hey, sorry for coming back with more suggestions. This is a big PR, and I keep missing small details. Functionally the PR works fine the way it is now. All remaining suggestions are just conforming to conventions.

pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
"druid.service" = "servicename";
};
};
jdk = mkPackageOptionMD pkgs "JDK" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
jdk = mkPackageOptionMD pkgs "JDK" {
jdk = mkPackageOption pkgs "JDK" {

nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/cluster/druid/default.nix Outdated Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
allowedTCPPorts = [ (attrByPath [ "druid.plaintextPort" ] 8083 cfg."${name}".config) ];
extraConfig.services.druid.historical.internalConfig."druid.segmentCache.locations" = ''
[${(concatMapStringsSep "," (
x: "{\"path\":\"" + x.path + "\",\"maxSize\":" + x.maxSize + "}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
x: "{\"path\":\"" + x.path + "\",\"maxSize\":" + x.maxSize + "}"
x: ''{"path":"${x.path}","maxSize":"${x.maxSize}"}''

[optional] get rid of all the escapes

@vsharathchandra vsharathchandra force-pushed the druid_nix_module branch 2 times, most recently from 41b5cb3 to 270f37a Compare March 23, 2024 08:07
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 25, 2024
Copy link
Contributor

@illustris illustris left a comment

Choose a reason for hiding this comment

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

Thanks!

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 28, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1565

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1748

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1806

Copy link
Contributor

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I've not looked atthe module yet, but here's some feedback for the package itself:

pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Show resolved Hide resolved
@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Jul 10, 2024
@eclairevoyant eclairevoyant dismissed their stale review July 10, 2024 19:22

changes addressed

pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Jul 18, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1860

pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/dr/druid/package.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/druid/default.nix Outdated Show resolved Hide resolved
@Aleksanaa
Copy link
Member

Aleksanaa commented Jul 27, 2024

You can ignore the "escaping with" part for now, it will be temporarily disabled

Edited: now force push to generate a new error report

@gador
Copy link
Member

gador commented Jul 27, 2024

Could you format the files with nixfmt?

@vsharathchandra vsharathchandra force-pushed the druid_nix_module branch 3 times, most recently from e77cdab to c363092 Compare July 28, 2024 06:17
(cfg.commonTmpDirs ++ tmpDirs);
};
networking.firewall.allowedTCPPorts =
mkIf ((attrByPath [ "openFirewall" ] false serviceOptions))

Choose a reason for hiding this comment

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

redundant parentheses

enable = true;
package = pkgs.mariadb;
initialDatabases = [{ name = "druid"; }];
initialScript = pkgs.writeText "mysql-init.sql" (''

Choose a reason for hiding this comment

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

redundant parentheses

passthru.override = args': testsForPackage (args // args');
};
testDruidCluster = { druidPackage, hadoopPackage, ... }:
pkgs.testers.nixosTest ({

Choose a reason for hiding this comment

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

redundant parentheses

inherit coreSite;
};
};
broker = { pkgs, ... }: {

Choose a reason for hiding this comment

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

definition pkgs is not used

};

};
coordinator = { pkgs, ... }: {

Choose a reason for hiding this comment

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

definition pkgs is not used

inherit coreSite;
};
};
mm = { pkgs, ... }: {

Choose a reason for hiding this comment

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

definition pkgs is not used

services.zookeeper.enable = true;
networking.firewall.allowedTCPPorts = [ 2181 ];
};
namenode = { pkgs, ... }: {

Choose a reason for hiding this comment

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

definition pkgs is not used

inherit coreSite;
};
};
overlord = { pkgs, ... }: {

Choose a reason for hiding this comment

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

definition pkgs is not used

</Configuration>
'';
log4j = pkgs.writeText "log4j2.xml" log4jConfig;
name = "hadoop-hdfs";

Choose a reason for hiding this comment

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

definition name is not used

log4j = pkgs.writeText "log4j2.xml" log4jConfig;
name = "hadoop-hdfs";
coreSite = { "fs.defaultFS" = "hdfs://namenode:8020"; };
package = pkgs.hadoop_3_2;

Choose a reason for hiding this comment

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

definition package is not used

Copy link

@nixf-tidy-review nixf-tidy-review left a comment

Choose a reason for hiding this comment

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

✅ Looks good!

@gador
Copy link
Member

gador commented Jul 28, 2024

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).

Result of nixpkgs-review pr 219942 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
2 packages built:
  • druid
  • nixpkgs-manual

The following issues got detected with the above build packages.
Please fix at least the ones listed with your changed packages:

nixpkgs-manual:

Package is missing maintainers.
If the package is using runCommand please make sure to inherit or list one or more maintainers.

@gador
Copy link
Member

gador commented Jul 28, 2024

nixosTests pass as well, all LGTM. Thanks for all the work you put in!

@gador gador merged commit 2c5b304 into NixOS:master Jul 28, 2024
27 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 8.has: tests This PR has tests 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.