You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/12/09 01:26:37 UTC
[kudu-CR] csd: two minor changes
Adar Dembo has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5435
Change subject: csd: two minor changes
......................................................................
csd: two minor changes
Revert the memory limit scaleFactor to the default value (1.0). 1.3 is
recommended for Java processes; not sure why Kudu was using that.
Convert the core dump directory to two separate role-level parameters. I
relented and did this once I saw that Impala also defines its core dump
directory in this way. In testing, the conversion was fine when the
service-level parameter wasn't set. When it was set, I saw that the
overridden value carried over to the roles even after the conversion
(good!). However, attempting to remove the so-called "role" overrides in the
UI yielded an error (bad!).
/api/v8/clusters/Cluster%201/services/KUDU-1/roles/KUDU-KUDU_MASTER-1/config?message=Updated%20service%20and%20role%20type%20configurations. \
{ "message" : "Could not find config to delete with template name: core_dump_directory" }
So this probably makes the conversion a non-starter, right?
Change-Id: I74b2fcb69fde9e0dbb232dba78db53cc1543b489
---
M java/kudu-csd/src/descriptor/service.sdl
1 file changed, 19 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/5435/1
--
To view, visit http://gerrit.cloudera.org:8080/5435
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I74b2fcb69fde9e0dbb232dba78db53cc1543b489
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
[kudu-CR] csd: two minor changes
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: csd: two minor changes
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5435/1//COMMIT_MSG
Commit Message:
Line 23: So this probably makes the conversion a non-starter, right?
> You can't share (template) names between service-level and role-level param
I'm comfortable with losing old service-level values, but only if we're really "losing" them; the patch as-is causes them to be both inherited and unrevertable.
If I change the parameter names, I think I get the desired behavior; the old value is lost, but the new parameters are isolated from one another as we wanted and seem to behave just fine. We'll just note that if you did override the service-level parameter, you'll lose the override in an upgrade.
--
To view, visit http://gerrit.cloudera.org:8080/5435
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I74b2fcb69fde9e0dbb232dba78db53cc1543b489
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #265
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] csd: two minor changes
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Anonymous Coward #265 has posted comments on this change.
Change subject: csd: two minor changes
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5435/1//COMMIT_MSG
Commit Message:
Line 23: So this probably makes the conversion a non-starter, right?
You can't share (template) names between service-level and role-level parameters, so this won't work, unfortunately.
You might be able to get them to work if you give them distinct names but the same config name. If you're lucky, you'll get "last one wins" semantics and the role-level one will appear last when both are set. You can then treat the role-level one as an override, with an empty default.
You'd have to test that works though, and we should officially support that on the CM side if you want to go that route (ie add a unit test that does this).
The safest thing to do, a method that I know will work, is to wait until Kudu is shipped with CM, then we can write some upgrade code to migrate your configs from service to role level.
You could also keep this patch as-is (role-level only, no service-level) if you're willing to just release note that the old service-level values will be lost. Wouldn't be the wildest thing to release note for the first GA release.
--
To view, visit http://gerrit.cloudera.org:8080/5435
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I74b2fcb69fde9e0dbb232dba78db53cc1543b489
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #265
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] csd: two minor changes
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5435
to look at the new patch set (#2).
Change subject: csd: two minor changes
......................................................................
csd: two minor changes
Revert the memory limit scaleFactor to the default value (1.0). 1.3 is
recommended for Java processes; not sure why Kudu was using that.
Convert the core dump directory to two separate role-level parameters. I
relented and did this once I saw that Impala also defines its core dump
directory in this way. In testing, the conversion was fine when the
service-level parameter wasn't set. When it was set, I saw that the
overridden value carried over to the roles even after the conversion
(good!). However, attempting to remove the so-called "role" overrides in the
UI yielded an error (bad!).
To work around this, I changed the names of the parameters (i.e. I prepended
either 'master' or 'tserver' to the names). This means any old value is lost
during an upgrade to the new CSD (a little bad), but that's better than not
being able to remove the value altogether.
Change-Id: I74b2fcb69fde9e0dbb232dba78db53cc1543b489
---
M java/kudu-csd/src/descriptor/service.sdl
1 file changed, 23 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/5435/2
--
To view, visit http://gerrit.cloudera.org:8080/5435
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I74b2fcb69fde9e0dbb232dba78db53cc1543b489
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #265
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] csd: two minor changes
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Anonymous Coward #265 has posted comments on this change.
Change subject: csd: two minor changes
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5435/2//COMMIT_MSG
Commit Message:
Line 20: To work around this, I changed the names of the parameters (i.e. I prepended
This works. Please file a JIRA to get these old values cleaned up in the CM Database if/when Kudu joins CM as a first-party CSD. I assume you'll also release note this.
--
To view, visit http://gerrit.cloudera.org:8080/5435
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I74b2fcb69fde9e0dbb232dba78db53cc1543b489
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #265
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] csd: two minor changes
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has submitted this change and it was merged.
Change subject: csd: two minor changes
......................................................................
csd: two minor changes
Revert the memory limit scaleFactor to the default value (1.0). 1.3 is
recommended for Java processes; not sure why Kudu was using that.
Convert the core dump directory to two separate role-level parameters. I
relented and did this once I saw that Impala also defines its core dump
directory in this way. In testing, the conversion was fine when the
service-level parameter wasn't set. When it was set, I saw that the
overridden value carried over to the roles even after the conversion
(good!). However, attempting to remove the so-called "role" overrides in the
UI yielded an error (bad!).
To work around this, I changed the names of the parameters (i.e. I prepended
either 'master' or 'tserver' to the names). This means any old value is lost
during an upgrade to the new CSD (a little bad), but that's better than not
being able to remove the value altogether.
Change-Id: I74b2fcb69fde9e0dbb232dba78db53cc1543b489
Reviewed-on: http://gerrit.cloudera.org:8080/5435
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
---
M java/kudu-csd/src/descriptor/service.sdl
1 file changed, 23 insertions(+), 15 deletions(-)
Approvals:
Jean-Daniel Cryans: Looks good to me, approved
Anonymous Coward #265: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5435
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I74b2fcb69fde9e0dbb232dba78db53cc1543b489
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #265
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] csd: two minor changes
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: csd: two minor changes
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5435
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I74b2fcb69fde9e0dbb232dba78db53cc1543b489
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #265
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No