Skip to content

2.27.0.0-b31

2.27.0.0-b31: [#26977] YSQL: remove YBBackupUtil from BasePgSQLTest
@jaki jaki tagged this 28 Apr 20:48 
Summary:
BasePgSQLTest has two methods with @Before junit annotation:
initYBBackupUtil and initPostgresBefore.  Ideally, we should avoid that
since [the ordering among those two methods is undefined][1].  One way
to do that is to have a single @Before method call these two methods.
However, upon closer look, YBBackupUtil appears to have no business
being in BasePgSQLTest since BasePgSQLTest is not entirely about
backups.  So adjust the objective to get rid of YBBackupUtil from
BasePgSQLTest.  This turns out to be non-trivial due to the piling on of
tech debt and/or hasty decisions made by the following commits:

- b598f62fc8cf5cf2d3a0719cae6f908249ce886c: to provide a utility method
  getTabletsForTable, there needed to be a way to run the yb-admin
  process.  Such code already existed with runProcess in YBBackupUtil.
  Rather than extract the generic code runProcess out of YBBackupUtil,
  this commit adds non-backup related runYbAdmin and getTabletsForTable
  into YBBackupUtil.
- cb8d9c59a8665ffbc9d5bd56d2eb7f6c2b3dd1fb: in order to count the number
  of tablets in a test in TestPgRegressTabletSplit, this commit wants to
  use getTabletsForTable from YBBackupUtil.  And rather than localize
  the damage to only borrow YBBackupUtil for this specific
  TestPgRegressTabletSplit, it brings it into BasePgSQLTest.

Since it is unavoidable, deal with those issues in this commit:

- Move getTabletsForTable, runYbAdmin, and runProcess out of
  YBBackupUtil.
  - Move getTabletsForTable to BaseMiniClusterTest.  I'm not fully
    satisfied with that, but it is a better place for the time being.
  - Move runYbAdmin to BaseMiniClusterTest as well since
    getTabletsForTable calls it.
  - Move runProcess to ProcessUtil.  It appears to be nearly identical
    to existing executeSimple, so add a todo comment to combine the
    methods.  Also, change the exceptions thrown in this method to no
    longer be YBBackupException and now be TimeoutException and
    IOException like in executeSimple.  This changes behavior of tests
    that have try-catch of YBBackupException: these two exceptions will
    now slip through uncaught.  These two exceptions shouldn't have been
    caught and excepted to begin with, so I treat this as fixing
    originally bad code.
- Rename BasePgSQLTest.getTabletsForTable to
  BasePgSQLTest.getTabletsForYsqlTable.  This is to avoid a name
  conflict with BaseMiniClusterTest.getTabletsForTable.  Also, get rid
  of the try-catch since the exceptions coming from getTabletsForTable
  are unexpected.
- TestPgRegressTabletSplit, TestYbBackup: update to use
  getTabletsForYsqlTable.

With all that done, remove BasePgSQLTest.initYBBackupUtil.

There is further potential to clean this area up, but I already spent
considerable time on this and feel it is a good stopping point.

[1]: https://junit.org/junit4/javadoc/latest/org/junit/Before.html
Jira: DB-16438

Test Plan:
Jenkins: java only

Close: #26977

Reviewers: jhe, loginov

Reviewed By: loginov

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D43087
Assets 2
Loading