-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
XDMoD 11.0 PHP 7.4 Changes #1806
Conversation
64f3a3a
to
ad594c7
Compare
ad594c7
to
aa045f5
Compare
bae4fb8
to
58129c1
Compare
0a89ec0
to
1e63e8e
Compare
b0e55d2
to
e101fbf
Compare
|
||
// There are instances where $firstPeriod is a bool not an array ( when $aggregationPeriodList is empty or at the end of the array) | ||
// the following code takes that into account. | ||
$periodSize = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 1
the appropriate default value for periodSize? If aggregationPeriodList is empty then why is periodSize not 0. Similarly if aggregationPeriodList is at the end then the periodSize would be 0 too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tldr;
I figured that setting $periodSize = 1
by default meant that $batchSlizeSize
wasn't (as) meaningless when determining whether or not to enable batch aggregation. Which I guess boils down to whether or not it matters if BatchAggregation is enabled when there are no dirty aggregation periods.
the not-tldr;
So $periodSize
's is strictly being used to determine whether or not to enable Batch Aggregation, the statement of note is:
// Default Values
$this->options->enable_batch_aggregation = true;
$batchSliceSize = $this->options->batch_aggregation_periods_per_batch = 10;
$this->options->batch_aggregation_min_num_periods = 25;
$this->options->batch_aggregation_max_num_days_per_batch = 300;
// Calculated Values
$numAggregationPeriods = count($aggregationPeriodList);
// Statement
$enableBatchAggregation =
$this->options->enable_batch_aggregation
&& $numAggregationPeriods >= $this->options->batch_aggregation_min_num_periods
&& ($periodSize * $batchSliceSize) <= $this->options->batch_aggregation_max_num_days_per_batch;
// Which basically boils down to:
true && $numAggregationPeriods >= 25 && ($periodSize * 10) <= 300;
// The result of which basically does some re-jiggering of of which / how tables are used in the aggregate query.
Which I guess is a really long way of saying... 0
works for me
docs/upgrade.md
Outdated
that are outside the normal installation of the rpm and `xdmod-upgrade`. Below you will find the different upgrade scenarios | ||
that exist with steps to accommodate all of them: | ||
|
||
### Server: EL7, XDMoD: 10.5, PHP: 7.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EL7 is not php 7.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is if you have 10.5 installed. But if not, then yeah it's 5.4 by default.
docs/upgrade.md
Outdated
Included in these new features is official support for the Rocky 8 operating system. This will allow organizations | ||
to migrate their XDMoD installations from the soon-to-be end-of-life CentOS 7 to a currently supported OS. The officially | ||
recommended process of migrating from a CentOS 7 XDMoD 10.0.X installation to an Rocky 8 XDMoD 10.5 installation is as follows: | ||
1. Install a fresh copy of XDMoD 10.5 on a new Rocky 8 server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these instructions should say:
if you have Centos 7 10.0 XDMoD then you need to update to Centos 7 10.5 XDMoD. Then once you have Centos 7 10.5 then you dump and restore to Rocky 8 10.5. Then you update XDMoD 10.5 to 11.0
You can't take a dump of the 10.0 database from Centos7 and expect it to work on Rocky 8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most definitely. Yeah, I should have updated the 10.0.x installation
to be 10.5.0
and had another full entry for 10.0.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this in the latest commits, let me know if what I've done works.
docs/upgrade.md
Outdated
### Server: EL8, XDMoD: 10.5, PHP: 7.2 | ||
The steps to accommodate this scenario are fairly straightforward: | ||
|
||
*NOTE: There may be some PHP Warning messages generated during this process, they can be safely ignored. Once the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to say what PHP warning are generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and when to expect to see such warning. - saying that all the warnings can be deregarded is likely to cause us problems when someone has a fatal error that they ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this w/ the specific warnings to look out for in the latest commit
docs/upgrade.md
Outdated
@@ -103,7 +155,7 @@ the recommended values listed in the [Configuration Guide][mysql-config]. | |||
Open XDMoD 11.0.0 is a major release that includes new features along with many | |||
enhancements and bug fixes. | |||
|
|||
Open XDMoD is now no longer bundled with any non-commercial licenses. The charting library used in Open XDMoD has changed from [Highcharts](https://www.highcharts.com/) to [Plotly JS](https://plotly.com/javascript/), an open source library. This transition removes the non-commercial license required from the Highcharts library. Please refer to the [license notices](notices.md) for more information about the open source licenses bundled with Open XDMoD. For more information please refer to [release notes](https://github.com/ubccr/xdmod/releases) for Open XDMoD 11.0. | |||
Open XDMoD is now no longer bundled with any non-commercial licenses. The charting library used in Open XDMoD has changed from [Highcharts](https://www.highcharts.com/) to [Plotly JS](https://plotly.com/javascript/), an open source library. This transition removes the non-commercial license required from the Highcharts library. Please refer to the [license notices](notices.md) for more information about the open source licenses bundled with Open XDMoD. For more information please refer to [release notes](https://github.com/ubccr/xdmod/releases) for Open XDMoD 11.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open XDMoD is now no longer bundled with any non-commercial licenses.
To be fair Open XDMoD has never been bundled with any licenses ever. It has, however, been bundled with software libraries that have licenses that have additional restrictions for commercial and governmental use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I certainly don't remember adding this bit, It sounds like something that would have come from the Plotly merge but I can certainly update it if we want to keep it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can you confirm this is correct? Seems odd to be adding an empty file here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would agree, I think this must have been generated while I was testing early on in development. I'll go ahead and remove this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been removed.
tests/ci/bootstrap.sh
Outdated
mysql_install_db --user mysql | ||
|
||
if [ -f /etc/my.cnf.d/mariadb-server.cnf.rpmsave ]; then | ||
mv /etc/my.cnf.d/mariadb-server.cnf.rpmsave /etc/my.cnf.d/mariadb-server.cnf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems unnecessary since the file is going to be overwritten 3 lines down on line 70.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it's removed in the latest
tests/ci/bootstrap.sh
Outdated
mv /etc/my.cnf.d/mariadb-server.cnf.rpmsave /etc/my.cnf.d/mariadb-server.cnf | ||
fi | ||
if [ -f /etc/my.cnf.d/mariadb-server.cnf ]; then | ||
>/etc/my.cnf.d/mariadb-server.cnf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why test for the file to exist before overwriting it? Under what conditions will the file not exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea, testing has been removed in the lates
tests/ci/bootstrap.sh
Outdated
datadir=/var/lib/mysql | ||
socket=/var/lib/mysql/mysql.sock | ||
log-error=/var/log/mariadb/mariadb.log | ||
pid-file=/run/mariadb/mariadb.pid" > /etc/my.cnf.d/mariadb-server.cnf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see line 70
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it's been updated in the latest.
In the interest of keeping things the same I'm adding the composer dep that will provide the PHPUnit `assertArraySubset` functionality that we were using prior to it being removed.
- Removed the SimpleSaml assertion install because this is now being handled in `build.json` - Removed the extraneous `|| true` from the mongodb step as it no longer returns a non-zero exit code. - Added `php-devel` to the PHP pre-reqs because these steps should be the same as what we detail in `upgrade.md` and users may not have `php-devel` installed.
Update per code review re using '*'.
This has been moved to `build.json`
I guess we do need the `|| true`.
Co-authored-by: Joe White <jpwhite4@buffalo.edu>
@@ -267,10 +267,14 @@ public function setSystemQuoteChar($char) | |||
|
|||
public function quote($identifier) | |||
{ | |||
// Don't quote the identifier if it's already been quoted | |||
// We can't quote non-strings so.... | |||
if (!is_string($identifier)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the safest way to handle this case? What scenarios exist where the quote function was called with a non string object? What was the object and what happened next?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably not, I've updated this in the latest commit to throw an Exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just some background, this code works fine in 7.2|7.4, but it was failing in 8.0 due to 8.X being more stringent about types.
Making it explicit what happens when an attempt to quote a non-string identifier is made. In PHP 7.4 this does not trigger, in PHP 8.* it will ( or at least it did ).
Note: This PR has been updated to require PHP 7.4 due to there being no support for 8.0. A separate PR has been made / is being tested for PHP 8.1
Description
.circleci/config.yml
:bin/acl-config
:fwrite()
has changed in PHP8.0, that change is reflected here.classes/DataWarehouse/Export/RealmManager.php
:export
: PHP8 enforces that you can't have optional function arguments ( arguments w/ defaults ) before required ones.classes/DataWarehouse/Visualization.php
:classes/ETL/Aggregator/pdoAggregator.php
:$firstPeriod
was assumed to be abool
value but it wasn't always. The updated code takes this into account.classes/ETL/DataEndpoint/aStructuredFile.php
:continue 2;
here generated PHP warnings.classes/ETL/Ingestor/HpcdbHostsIngestor.php
:transform()
function did not fully conform to the parent definintion, now it does.classes/ETL/aRdbmsDestinationAction.php
:if
statement failed when it should pass due to intermediary array entries not being present / not having the expected type. The updatedif
statement behaves as expected.classes/Realm/GroupBy.php
:classes/ETL/DataEndpoint/aStructuredFile.php
, warnings were generated for these lines.composer.json
:tests/artifacts/xdmod/**
:sem_avg_cpu_hours
values to include another decimal point of value to reflect what PHP8 provides.tests/ci/bootstrap.sh
tests/ci/samlSetup.sh
tests/[component|integration|regression|unit]/**
:Motivation and Context
We'd like to move with the times and start using officially supported versions of PHP. This is the first step on that path.
Tests performed
All Automated Tests
Checklist: