Re: Review Request 69677: RANGER-2287: Improve and optimize db_setup.py file code
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69677/#review212238 --- Ship it! Ship It! - Velmurugan Periasamy On Jan. 22, 2019, 1:47 p.m., Pradeep Agrawal wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69677/ > --- > > (Updated Jan. 22, 2019, 1:47 p.m.) > > > Review request for ranger, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, > Nikhil P, Ramesh Mani, and Velmurugan Periasamy. > > > Bugs: RANGER-2287 > https://issues.apache.org/jira/browse/RANGER-2287 > > > Repository: ranger > > > Description > --- > > **Problem Statement:** There are lot of repeated code in db_setup.py which > can be removed which shall help developers to make any changes in db_setup.py > in future. > > **Proposed Solution:** Proposed patch shall remove the db setup methods of > each db flavor and shall use a single method for a specific work for each db > flavor. Based on the db flavor, config values shall be populated and handled > in the code after this patch. > > > Diffs > - > > security-admin/scripts/db_setup.py f1223b38c > > > Diff: https://reviews.apache.org/r/69677/diff/3/ > > > Testing > --- > > **Use Cases covered for all the db flavors:** > *1. Fresh installation(Ranger 2.0):* Tested patch with fresh installation of > ranger admin. > *2. Upgrade(from 0.7 to 2.0):* Installed Ranger from 0.7 branch and used same > db config on Ranger 2.0 installation config and run the setup.sh. Ranger was > upgraded successfully. > > > Thanks, > > Pradeep Agrawal > >
Re: Review Request 69677: RANGER-2287: Improve and optimize db_setup.py file code
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69677/#review212231 --- Ship it! Ship It! - Zsombor Gegesy On Jan. 22, 2019, 1:47 p.m., Pradeep Agrawal wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69677/ > --- > > (Updated Jan. 22, 2019, 1:47 p.m.) > > > Review request for ranger, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, > Nikhil P, Ramesh Mani, and Velmurugan Periasamy. > > > Bugs: RANGER-2287 > https://issues.apache.org/jira/browse/RANGER-2287 > > > Repository: ranger > > > Description > --- > > **Problem Statement:** There are lot of repeated code in db_setup.py which > can be removed which shall help developers to make any changes in db_setup.py > in future. > > **Proposed Solution:** Proposed patch shall remove the db setup methods of > each db flavor and shall use a single method for a specific work for each db > flavor. Based on the db flavor, config values shall be populated and handled > in the code after this patch. > > > Diffs > - > > security-admin/scripts/db_setup.py f1223b38c > > > Diff: https://reviews.apache.org/r/69677/diff/3/ > > > Testing > --- > > **Use Cases covered for all the db flavors:** > *1. Fresh installation(Ranger 2.0):* Tested patch with fresh installation of > ranger admin. > *2. Upgrade(from 0.7 to 2.0):* Installed Ranger from 0.7 branch and used same > db config on Ranger 2.0 installation config and run the setup.sh. Ranger was > upgraded successfully. > > > Thanks, > > Pradeep Agrawal > >
Re: Review Request 69677: RANGER-2287: Improve and optimize db_setup.py file code
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69677/ --- (Updated Jan. 22, 2019, 1:47 p.m.) Review request for ranger, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Nikhil P, Ramesh Mani, and Velmurugan Periasamy. Changes --- Updated patch after one more round of testing Bugs: RANGER-2287 https://issues.apache.org/jira/browse/RANGER-2287 Repository: ranger Description --- **Problem Statement:** There are lot of repeated code in db_setup.py which can be removed which shall help developers to make any changes in db_setup.py in future. **Proposed Solution:** Proposed patch shall remove the db setup methods of each db flavor and shall use a single method for a specific work for each db flavor. Based on the db flavor, config values shall be populated and handled in the code after this patch. Diffs (updated) - security-admin/scripts/db_setup.py f1223b38c Diff: https://reviews.apache.org/r/69677/diff/3/ Changes: https://reviews.apache.org/r/69677/diff/2-3/ Testing --- **Use Cases covered for all the db flavors:** *1. Fresh installation(Ranger 2.0):* Tested patch with fresh installation of ranger admin. *2. Upgrade(from 0.7 to 2.0):* Installed Ranger from 0.7 branch and used same db config on Ranger 2.0 installation config and run the setup.sh. Ranger was upgraded successfully. Thanks, Pradeep Agrawal
Re: Review Request 69677: RANGER-2287: Improve and optimize db_setup.py file code
> On Jan. 8, 2019, 11:48 a.m., Zsombor Gegesy wrote:
> > It's great news, that you could delete thousands of lines of repetitive
> > code, however you could achieve more, if instead of putting everything into
> > one class, and put
> > '''
> > if self.XA_DB_FLAVOR == DB_MYSQL:
> >...
> > elif self.XA_DB_FLAVOR == DB_POSTGRES:
> >...
> > '''
> >
> > You can write
> >self.do_something(...)
> >
> > and implement do_something differently in the MySQL/PostgreSQL/Oracle
> > specific adapter class
>
> Pradeep Agrawal wrote:
> There shall be too many self.do_something(...) function I have to write
> which shall look like the previous code. Can you review it once again and let
> me know with few examples.
>
> Zsombor Gegesy wrote:
> Maybe you can add:
> '''
> def execute_query(self, query):
> ''' Execute query and return the output as a string '''
> get_cmd = self.get_jisql_cmd(self.db_user, self.db_password,
> self.db_name)
> if is_unix:
> full_command = get_cmd + " -query \"" + query + "\""
> elif os_name == "WINDOWS":
> full_command = get_cmd + " -query \"" + query + "\" -c ;"
> else:
> raise Exception("This OS is not supported!")
> jisql_log(full_command, self.db_password)
> output = check_output(query)
> return output
>
> def execute_update(self, update):
> ''' Execute the update query and return the error code'''
> get_cmd = self.get_jisql_cmd(self.db_user, self.db_password,
> self.db_name)
> if is_unix:
> full_command = get_cmd + " -query \"" + update + "\""
> jisql_log(full_command, self.db_password)
> return subprocess.call(shlex.split(query))
> elif os_name == "WINDOWS":
> full_command = get_cmd + " -query \"" + update + "\" -c ;"
> jisql_log(full_command, self.db_password)
> ret = subprocess.call(query)
> raise Exception("This OS is not supported!")
> '''
>
> So you can get rid of lot's of repeating code around to support Windows.
>
> And for the db changes, I would imagine something like this:
>
> '''
> class BaseDB(object):
>
> @abstractmethod
> def get_stale_patch_query(self, version, client_host,
> stalePatchEntryHoldTimeInMinutes):
> pass
>
>
> class MysqlConf(BaseDB):
>
> def get_stale_patch_query(self, version, client_host,
> stalePatchEntryHoldTimeInMinutes):
> return "select version from x_db_version_h where version = '%s'
> and active = 'N' and updated_by='%s' and
> TIMESTAMPDIFF(MINUTE,inst_at,CURRENT_TIMESTAMP)>=%s;" % (version,
> client_host, stalePatchEntryHoldTimeInMinutes)
>
> '''
>
>
> So you can write:
>
> '''
> output =
> self.execute_query(self.get_stale_patch_query(version,client_host,stalePatchEntryHoldTimeInMinutes))
> ...
> '''
>
> What do you think, does it makes sense?
Can you please review the updated patch again.
- Pradeep
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69677/#review211760
---
On Jan. 15, 2019, 12:55 p.m., Pradeep Agrawal wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69677/
> ---
>
> (Updated Jan. 15, 2019, 12:55 p.m.)
>
>
> Review request for ranger, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh,
> Nikhil P, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-2287
> https://issues.apache.org/jira/browse/RANGER-2287
>
>
> Repository: ranger
>
>
> Description
> ---
>
> **Problem Statement:** There are lot of repeated code in db_setup.py which
> can be removed which shall help developers to make any changes in db_setup.py
> in future.
>
> **Proposed Solution:** Proposed patch shall remove the db setup methods of
> each db flavor and shall use a single method for a specific work for each db
> flavor. Based on the db flavor, config values shall be populated and handled
> in the code after this patch.
>
>
> Diffs
> -
>
> security-admin/scripts/db_setup.py f1223b38c
>
>
> Diff: https://reviews.apache.org/r/69677/diff/2/
>
>
> Testing
> ---
>
> **Use Cases covered for all the db flavors:**
> *1. Fresh installation(Ranger 2.0):* Tested patch with fresh installation of
> ranger admin.
> *2. Upgrade(from 0.7 to 2.0):* Installed Ranger from 0.7 branch and used same
> db config on Ranger 2.0 installation config and run the setup.sh. Ranger was
> upgraded successfully.
>
>
> Thanks,
>
> Pradeep Agrawal
>
>
Re: Review Request 69677: RANGER-2287: Improve and optimize db_setup.py file code
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69677/ --- (Updated Jan. 15, 2019, 12:55 p.m.) Review request for ranger, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Nikhil P, Ramesh Mani, and Velmurugan Periasamy. Changes --- Updated patch as per the feedback. Bugs: RANGER-2287 https://issues.apache.org/jira/browse/RANGER-2287 Repository: ranger Description --- **Problem Statement:** There are lot of repeated code in db_setup.py which can be removed which shall help developers to make any changes in db_setup.py in future. **Proposed Solution:** Proposed patch shall remove the db setup methods of each db flavor and shall use a single method for a specific work for each db flavor. Based on the db flavor, config values shall be populated and handled in the code after this patch. Diffs (updated) - security-admin/scripts/db_setup.py f1223b38c Diff: https://reviews.apache.org/r/69677/diff/2/ Changes: https://reviews.apache.org/r/69677/diff/1-2/ Testing --- **Use Cases covered for all the db flavors:** *1. Fresh installation(Ranger 2.0):* Tested patch with fresh installation of ranger admin. *2. Upgrade(from 0.7 to 2.0):* Installed Ranger from 0.7 branch and used same db config on Ranger 2.0 installation config and run the setup.sh. Ranger was upgraded successfully. Thanks, Pradeep Agrawal
Re: Review Request 69677: RANGER-2287: Improve and optimize db_setup.py file code
> On Jan. 8, 2019, 11:48 a.m., Zsombor Gegesy wrote:
> > It's great news, that you could delete thousands of lines of repetitive
> > code, however you could achieve more, if instead of putting everything into
> > one class, and put
> > '''
> > if self.XA_DB_FLAVOR == DB_MYSQL:
> >...
> > elif self.XA_DB_FLAVOR == DB_POSTGRES:
> >...
> > '''
> >
> > You can write
> >self.do_something(...)
> >
> > and implement do_something differently in the MySQL/PostgreSQL/Oracle
> > specific adapter class
>
> Pradeep Agrawal wrote:
> There shall be too many self.do_something(...) function I have to write
> which shall look like the previous code. Can you review it once again and let
> me know with few examples.
Maybe you can add:
'''
def execute_query(self, query):
''' Execute query and return the output as a string '''
get_cmd = self.get_jisql_cmd(self.db_user, self.db_password, self.db_name)
if is_unix:
full_command = get_cmd + " -query \"" + query + "\""
elif os_name == "WINDOWS":
full_command = get_cmd + " -query \"" + query + "\" -c ;"
else:
raise Exception("This OS is not supported!")
jisql_log(full_command, self.db_password)
output = check_output(query)
return output
def execute_update(self, update):
''' Execute the update query and return the error code'''
get_cmd = self.get_jisql_cmd(self.db_user, self.db_password, self.db_name)
if is_unix:
full_command = get_cmd + " -query \"" + update + "\""
jisql_log(full_command, self.db_password)
return subprocess.call(shlex.split(query))
elif os_name == "WINDOWS":
full_command = get_cmd + " -query \"" + update + "\" -c ;"
jisql_log(full_command, self.db_password)
ret = subprocess.call(query)
raise Exception("This OS is not supported!")
'''
So you can get rid of lot's of repeating code around to support Windows.
And for the db changes, I would imagine something like this:
'''
class BaseDB(object):
@abstractmethod
def get_stale_patch_query(self, version, client_host,
stalePatchEntryHoldTimeInMinutes):
pass
class MysqlConf(BaseDB):
def get_stale_patch_query(self, version, client_host,
stalePatchEntryHoldTimeInMinutes):
return "select version from x_db_version_h where version = '%s' and
active = 'N' and updated_by='%s' and
TIMESTAMPDIFF(MINUTE,inst_at,CURRENT_TIMESTAMP)>=%s;" % (version, client_host,
stalePatchEntryHoldTimeInMinutes)
'''
So you can write:
'''
output =
self.execute_query(self.get_stale_patch_query(version,client_host,stalePatchEntryHoldTimeInMinutes))
...
'''
What do you think, does it makes sense?
- Zsombor
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69677/#review211760
---
On Jan. 7, 2019, 6:37 a.m., Pradeep Agrawal wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69677/
> ---
>
> (Updated Jan. 7, 2019, 6:37 a.m.)
>
>
> Review request for ranger, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh,
> Nikhil P, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-2287
> https://issues.apache.org/jira/browse/RANGER-2287
>
>
> Repository: ranger
>
>
> Description
> ---
>
> **Problem Statement:** There are lot of repeated code in db_setup.py which
> can be removed which shall help developers to make any changes in db_setup.py
> in future.
>
> **Proposed Solution:** Proposed patch shall remove the db setup methods of
> each db flavor and shall use a single method for a specific work for each db
> flavor. Based on the db flavor, config values shall be populated and handled
> in the code after this patch.
>
>
> Diffs
> -
>
> security-admin/scripts/db_setup.py f1223b38c
>
>
> Diff: https://reviews.apache.org/r/69677/diff/1/
>
>
> Testing
> ---
>
> **Use Cases covered for all the db flavors:**
> *1. Fresh installation(Ranger 2.0):* Tested patch with fresh installation of
> ranger admin.
> *2. Upgrade(from 0.7 to 2.0):* Installed Ranger from 0.7 branch and used same
> db config on Ranger 2.0 installation config and run the setup.sh. Ranger was
> upgraded successfully.
>
>
> Thanks,
>
> Pradeep Agrawal
>
>
Re: Review Request 69677: RANGER-2287: Improve and optimize db_setup.py file code
> On Jan. 8, 2019, 11:48 a.m., Zsombor Gegesy wrote: > > It's great news, that you could delete thousands of lines of repetitive > > code, however you could achieve more, if instead of putting everything into > > one class, and put > > ''' > > if self.XA_DB_FLAVOR == DB_MYSQL: > >... > > elif self.XA_DB_FLAVOR == DB_POSTGRES: > >... > > ''' > > > > You can write > >self.do_something(...) > > > > and implement do_something differently in the MySQL/PostgreSQL/Oracle > > specific adapter class There shall be too many self.do_something(...) function I have to write which shall look like the previous code. Can you review it once again and let me know with few examples. - Pradeep --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69677/#review211760 --- On Jan. 7, 2019, 6:37 a.m., Pradeep Agrawal wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69677/ > --- > > (Updated Jan. 7, 2019, 6:37 a.m.) > > > Review request for ranger, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, > Nikhil P, Ramesh Mani, and Velmurugan Periasamy. > > > Bugs: RANGER-2287 > https://issues.apache.org/jira/browse/RANGER-2287 > > > Repository: ranger > > > Description > --- > > **Problem Statement:** There are lot of repeated code in db_setup.py which > can be removed which shall help developers to make any changes in db_setup.py > in future. > > **Proposed Solution:** Proposed patch shall remove the db setup methods of > each db flavor and shall use a single method for a specific work for each db > flavor. Based on the db flavor, config values shall be populated and handled > in the code after this patch. > > > Diffs > - > > security-admin/scripts/db_setup.py f1223b38c > > > Diff: https://reviews.apache.org/r/69677/diff/1/ > > > Testing > --- > > **Use Cases covered for all the db flavors:** > *1. Fresh installation(Ranger 2.0):* Tested patch with fresh installation of > ranger admin. > *2. Upgrade(from 0.7 to 2.0):* Installed Ranger from 0.7 branch and used same > db config on Ranger 2.0 installation config and run the setup.sh. Ranger was > upgraded successfully. > > > Thanks, > > Pradeep Agrawal > >
Re: Review Request 69677: RANGER-2287: Improve and optimize db_setup.py file code
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69677/#review211760 --- It's great news, that you could delete thousands of lines of repetitive code, however you could achieve more, if instead of putting everything into one class, and put ''' if self.XA_DB_FLAVOR == DB_MYSQL: ... elif self.XA_DB_FLAVOR == DB_POSTGRES: ... ''' You can write self.do_something(...) and implement do_something differently in the MySQL/PostgreSQL/Oracle specific adapter class - Zsombor Gegesy On Jan. 7, 2019, 6:37 a.m., Pradeep Agrawal wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69677/ > --- > > (Updated Jan. 7, 2019, 6:37 a.m.) > > > Review request for ranger, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, > Nikhil P, Ramesh Mani, and Velmurugan Periasamy. > > > Bugs: RANGER-2287 > https://issues.apache.org/jira/browse/RANGER-2287 > > > Repository: ranger > > > Description > --- > > **Problem Statement:** There are lot of repeated code in db_setup.py which > can be removed which shall help developers to make any changes in db_setup.py > in future. > > **Proposed Solution:** Proposed patch shall remove the db setup methods of > each db flavor and shall use a single method for a specific work for each db > flavor. Based on the db flavor, config values shall be populated and handled > in the code after this patch. > > > Diffs > - > > security-admin/scripts/db_setup.py f1223b38c > > > Diff: https://reviews.apache.org/r/69677/diff/1/ > > > Testing > --- > > **Use Cases covered for all the db flavors:** > *1. Fresh installation(Ranger 2.0):* Tested patch with fresh installation of > ranger admin. > *2. Upgrade(from 0.7 to 2.0):* Installed Ranger from 0.7 branch and used same > db config on Ranger 2.0 installation config and run the setup.sh. Ranger was > upgraded successfully. > > > Thanks, > > Pradeep Agrawal > >
