> 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
>
>
