Design for refactoring PBS database code

Here is the design page for refactoring PBS database code. I am proposing to make databse code to a dynamic library and PBS will talk to this library over bunch of APIs.

https://pbspro.atlassian.net/wiki/spaces/PD/pages/1524563969/DB+Refactor+Design.

Thanks.

Thanks for posting this Ashwath. The doc looks good! Some comments:

  • “PBS_EXEC/sbin/pbs_dataservice <start|stop|status>”: Can you please mention here what “status” will return?
  • “PBS_EXEC/sbin/pbs_ds_password -r | -C <user_name>”:
    • Can you please explain what the options -r and -C do? Are there any other options?
    • Does this script configure the underlying db? If yes, how will it work for different kinds of db? Will one have to modify this file to add support for a new db?
    • Does “user_name” need to be an existing user or can the script actually create a new user?
  • Just a thought, right now it seems like if one wanted to replace the db service from postgres to, say, redis, then they’d need their own version of pbs_dataservice, pbs_dataservice and pbs_db_utility, along with the libdb.so. Why not instead move the functionality of these additional scripts also to libdb.so and convert these scripts into generic programs instead which link with libdb? That way, one will just have to replace postgres libdb.so with redis libdb.so in order to completely replace postgres with redis.
  • PBS_EXEC/include/pbs_db.h - can you please provide details about all the members of these structs?
  • pbs_db_connect:
    • how about renaming pbs_data_service_host and pbs_data_service_port to pbs_ds_host and pbs_ds_port instead?
    • Should this interface take a ‘timeout’ argument?
  • pbs_db_disconnect:
    • You’ve mentioned that this function will also stop the db. Since there’s a separate function for stopping the db, i think you should remove that from here.
    • this function takes a “char **db_msg” while connect takes a “char *err_msg”, can we make them consistent? If this is a return pointer then I think it will need to be a double pointer anyways which points to a single string.
    • “db_msg[out] : Connection logs/messages generated by libdb”: I think any log messages generated by libdb should be logged separately. How about a new “db_logs” directory similar to other logs in pbs?
  • get_db_errmsg: It seems like most interfaces return an error message directly anyways. So, is this really needed? If the interfaces returned an error code instead of an error message then this interface would make more sense.
  • pbs_start_db, pbs_stop_db and pbs_status_db: Now that I think about it, pbs_dataservice is going to be the way one would start, status and stop the db right? I don’t think we need these functions, what do you think?

Thanks for the comments.

  • “PBS_EXEC/sbin/pbs_dataservice <start|stop|status>”: Can you please mention here what “status” will return?

The status option will return the status of the database instance.

  • “PBS_EXEC/sbin/pbs_ds_password -r | -C <user_name>”:
  • Can you please explain what the options -r and -C do? Are there any other options?
  • Does this script configure the underlying db? If yes, how will it work for different kinds of db? Will one have to modify this file to add support for a new db?
  • Does “user_name” need to be an existing user or can the script actually create a new user?

I updated the page with info on the options. -r is to set a random password to the current database user. -C can create or change the database user to a new one.

  • Just a thought, right now it seems like if one wanted to replace the db service from postgres to, say, redis, then they’d need their own version of pbs_dataservice , pbs_dataservice and pbs_db_utility , along with the libdb.so. Why not instead move the functionality of these additional scripts also to libdb.so and convert these scripts into generic programs instead which link with libdb? That way, one will just have to replace postgres libdb.so with redis libdb.so in order to completely replace postgres with redis.

pbs_ds_password is already a binary executable. Since pbs_dataservice and utility scripts will be mostly used by pre-configure scripts of initial pbs startup, it makes sense to retain them as scripts. What do you think?

  • PBS_EXEC/include/pbs_db.h - can you please provide details about all the members of these structs?

Added structure info to the page.

  • pbs_db_connect :
  • how about renaming pbs_data_service_host and pbs_data_service_port to pbs_ds_host and pbs_ds_port instead?

Updated.

  • Should this interface take a ‘timeout’ argument?

Yes, this will make sure PBS server won’t wait indefinitely trying to connect to database.

  • pbs_db_disconnect :
  • You’ve mentioned that this function will also stop the db. Since there’s a separate function for stopping the db, i think you should remove that from here.
  • this function takes a “char **db_msg” while connect takes a “char *err_msg”, can we make them consistent? If this is a return pointer then I think it will need to be a double pointer anyways which points to a single string.

Yes, corrected it now. I removed failcode. I guess we can rely on err_msg to capture both system and db error messages.

  • “db_msg[out] : Connection logs/messages generated by libdb”: I think any log messages generated by libdb should be logged separately. How about a new “db_logs” directory similar to other logs in pbs?

PBS server is the one who is talking to libdb so it only makes sense we log the messages to server log. No?

  • get_db_errmsg : It seems like most interfaces return an error message directly anyways. So, is this really needed? If the interfaces returned an error code instead of an error message then this interface would make more sense.
  • pbs_start_db, pbs_stop_db and pbs_status_db : Now that I think about it, pbs_dataservice is going to be the way one would start, status and stop the db right? I don’t think we need these functions, what do you think?

After reading your comment, yes it makes sense. The server internally uses data service script to start/stop the database.

Let me know your opinion on the changes I have made. Thanks.