You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@fluo.apache.org by GitBox <gi...@apache.org> on 2020/01/29 01:53:19 UTC
[GitHub] [fluo-muchos] arvindshmicrosoft opened a new pull request #315: Add
conditions for Azure specific tasks
arvindshmicrosoft opened a new pull request #315: Add conditions for Azure specific tasks
URL: https://github.com/apache/fluo-muchos/pull/315
Fixes #314
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fluo-muchos] arvindshmicrosoft commented on a change in pull
request #315: Add conditions for Azure specific tasks
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on a change in pull request #315: Add conditions for Azure specific tasks
URL: https://github.com/apache/fluo-muchos/pull/315#discussion_r372188815
##########
File path: ansible/accumulo.yml
##########
@@ -30,11 +30,11 @@
- hosts: all:!{{ azure_proxy_host }}
tasks:
- import_tasks: roles/accumulo/tasks/add-adlsgen2.yml
- when: accumulo_major_version == '2' and use_adlsg2 == True
+ when: cluster_type == 'azure' and accumulo_major_version == '2' and use_adlsg2 == True
Review comment:
Agreed, I am reviewing this specific variable all over the board and will push up a commit. Will also review and refactor other such expressions in a separate PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fluo-muchos] ctubbsii commented on a change in pull request #315:
Add conditions for Azure specific tasks
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #315: Add conditions for Azure specific tasks
URL: https://github.com/apache/fluo-muchos/pull/315#discussion_r372167100
##########
File path: ansible/roles/accumulo/templates/accumulo.properties
##########
@@ -43,8 +43,8 @@ tserver.server.threads.minimum=64
## The maximum size for each write-ahead log
tserver.walog.max.size=512M
-{% if use_adlsg2 == True %}
+{% if cluster_type == 'azure' and use_adlsg2 == True %}
general.volume.chooser=org.apache.accumulo.server.fs.PreferredVolumeChooser
general.custom.volume.preferred.default={{ instance_volumes_preferred }}
general.custom.volume.preferred.logger={{ hdfs_root }}/accumulo
-{% endif %}
+{% endif %}
Review comment:
This change unnecessarily deletes the end-of-line marker. It's not a huge deal, but it's generally good practice to terminate every line with a line-ending sequence (this project uses Unix style line endings), rather than rely on the EOF to substitute for EOL. This is something to look out for when submitting patches produced by a badly-behaving text editor. See https://stackoverflow.com/a/5813359/196405 for more discussion about this as it pertains to 'unified diff'-style patches, git, C standards, etc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fluo-muchos] ctubbsii commented on a change in pull request #315:
Add conditions for Azure specific tasks
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #315: Add conditions for Azure specific tasks
URL: https://github.com/apache/fluo-muchos/pull/315#discussion_r372201037
##########
File path: ansible/accumulo.yml
##########
@@ -30,11 +30,11 @@
- hosts: all:!{{ azure_proxy_host }}
tasks:
- import_tasks: roles/accumulo/tasks/add-adlsgen2.yml
- when: accumulo_major_version == '2' and use_adlsg2 == True
+ when: cluster_type == 'azure' and accumulo_major_version == '2' and use_adlsg2 == True
Review comment:
Sounds good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fluo-muchos] arvindshmicrosoft commented on a change in pull
request #315: Add conditions for Azure specific tasks
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on a change in pull request #315: Add conditions for Azure specific tasks
URL: https://github.com/apache/fluo-muchos/pull/315#discussion_r372188130
##########
File path: ansible/roles/accumulo/templates/accumulo.properties
##########
@@ -43,8 +43,8 @@ tserver.server.threads.minimum=64
## The maximum size for each write-ahead log
tserver.walog.max.size=512M
-{% if use_adlsg2 == True %}
+{% if cluster_type == 'azure' and use_adlsg2 == True %}
general.volume.chooser=org.apache.accumulo.server.fs.PreferredVolumeChooser
general.custom.volume.preferred.default={{ instance_volumes_preferred }}
general.custom.volume.preferred.logger={{ hdfs_root }}/accumulo
-{% endif %}
+{% endif %}
Review comment:
Got it! Will keep this in mind going forward.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fluo-muchos] arvindshmicrosoft merged pull request #315: Add
conditions for Azure specific tasks
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft merged pull request #315: Add conditions for Azure specific tasks
URL: https://github.com/apache/fluo-muchos/pull/315
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fluo-muchos] ctubbsii commented on a change in pull request #315:
Add conditions for Azure specific tasks
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #315: Add conditions for Azure specific tasks
URL: https://github.com/apache/fluo-muchos/pull/315#discussion_r372166170
##########
File path: ansible/accumulo.yml
##########
@@ -30,11 +30,11 @@
- hosts: all:!{{ azure_proxy_host }}
tasks:
- import_tasks: roles/accumulo/tasks/add-adlsgen2.yml
- when: accumulo_major_version == '2' and use_adlsg2 == True
+ when: cluster_type == 'azure' and accumulo_major_version == '2' and use_adlsg2 == True
Review comment:
`use_adlsg2 == True` still feels a bit redundant to me. It seems likely that just `use_adlsg2` would work, but I haven't tried it (same comment for the other occurrences of this also):
```suggestion
when: cluster_type == 'azure' and accumulo_major_version == '2' and use_adlsg2
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fluo-muchos] arvindshmicrosoft commented on issue #315: Add
conditions for Azure specific tasks
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on issue #315: Add conditions for Azure specific tasks
URL: https://github.com/apache/fluo-muchos/pull/315#issuecomment-579618923
> Nice! I assume you have tested the proposed change I made regarding the boolean equality comparisons (and you're not just taking my word for it... because I definitely didn't test it myself... 😺).
>
> This looks good to me.
Thank you, Christopher. Yes, I ran tests for both 'azure' (with and without ADLS Gen2) and also the 'existing' cluster types before I pushed these up.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fluo-muchos] ctubbsii commented on a change in pull request #315:
Add conditions for Azure specific tasks
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #315: Add conditions for Azure specific tasks
URL: https://github.com/apache/fluo-muchos/pull/315#discussion_r372167100
##########
File path: ansible/roles/accumulo/templates/accumulo.properties
##########
@@ -43,8 +43,8 @@ tserver.server.threads.minimum=64
## The maximum size for each write-ahead log
tserver.walog.max.size=512M
-{% if use_adlsg2 == True %}
+{% if cluster_type == 'azure' and use_adlsg2 == True %}
general.volume.chooser=org.apache.accumulo.server.fs.PreferredVolumeChooser
general.custom.volume.preferred.default={{ instance_volumes_preferred }}
general.custom.volume.preferred.logger={{ hdfs_root }}/accumulo
-{% endif %}
+{% endif %}
Review comment:
This change unnecessarily deletes the end-of-line marker. It's not a huge deal, but it's generally good practice to terminate every line with a line-ending sequence (this project uses Unix style line endings), rather than rely on the EOF to substitute for EOL. This is something to look out for when submitting patches produced by a badly-behaving text editor. See https://stackoverflow.com/a/729795/196405, https://stackoverflow.com/a/5813359/196405, https://unix.stackexchange.com/a/18789 for more discussion about this as it pertains to 'unified diff'-style patches, git, C standards, POSIX, etc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services