x/gearman-plugin
Code Issues Proposed changes

Update patch set 4

Browse Source 
Patch Set 4: Code-Review-1

(5 comments)

Patch-set: 4
Label: Code-Review=-1
This commit is contained in:
Gerrit User 6987 2015-08-11 22:24:53 +00:00 committed by Gerrit Code Review
parent 576476a347
commit 5e3690d15f
1 changed files with 89 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

89
be8d107f35d511cb91c36b82a6c5ba4684ea19be Normal file
View File

@ -0,0 +1,89 @@
{
"comments": [
{
"key": {
"uuid": "1a4dcd0f_fa1a6e6e",
"filename": "src/main/java/hudson/plugins/gearman/CustomGearmanFunctionFactory.java",
"patchSetId": 4
},
"lineNbr": 47,
"author": {
"id": 6987
},
"writtenOn": "2015-08-11T22:24:53Z",
"side": 1,
"message": "So jenkins has two concepts of a \u0027label\u0027, the label that\u0027s on the project (or job) and the label that\u0027s on the slave (or node). Could you please make that distinction by choosing a better name for this variable here and in the other files?",
"revId": "be8d107f35d511cb91c36b82a6c5ba4684ea19be",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "1a4dcd0f_bf0618cc",
"filename": "src/main/java/hudson/plugins/gearman/ExecutorWorkerThread.java",
"patchSetId": 4
},
"lineNbr": 156,
"author": {
"id": 6987
},
"writtenOn": "2015-08-11T22:24:53Z",
"side": 1,
"message": "This \u0027label\u0027 object is the project\u0027s (or job\u0027s) label not the node\u0027s label. Also the comments seem to indicate that want to register _without_ the label info here. This is the case where we want to register both \u0027build:foo\u0027 and \u0027build:foo:slave1\u0027 for nodes that have matching job labels.",
"revId": "be8d107f35d511cb91c36b82a6c5ba4684ea19be",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "1a4dcd0f_bfd13859",
"filename": "src/main/java/hudson/plugins/gearman/ExecutorWorkerThread.java",
"patchSetId": 4
},
"lineNbr": 160,
"author": {
"id": 6987
},
"writtenOn": "2015-08-11T22:24:53Z",
"side": 1,
"message": "labelAtom.getDisplayName is called here as well so would it be possible to assign to a variable to re-use?",
"revId": "be8d107f35d511cb91c36b82a6c5ba4684ea19be",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "1a4dcd0f_5adcc272",
"filename": "src/main/java/hudson/plugins/gearman/StartJobWorker.java",
"patchSetId": 4
},
"lineNbr": 81,
"author": {
"id": 6987
},
"writtenOn": "2015-08-11T22:24:53Z",
"side": 1,
"message": "again, unclear which label this represents.",
"revId": "be8d107f35d511cb91c36b82a6c5ba4684ea19be",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "1a4dcd0f_5f24b4c3",
"filename": "src/main/java/hudson/plugins/gearman/StartJobWorker.java",
"patchSetId": 4
},
"lineNbr": 95,
"author": {
"id": 6987
},
"writtenOn": "2015-08-11T22:24:53Z",
"side": 1,
"message": "I think i prefer the consistency of always having the label in the data. I guess i would just set it to empty string if it\u0027s null.",
"revId": "be8d107f35d511cb91c36b82a6c5ba4684ea19be",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
}
]
}