Page MenuHomePhabricator

Deprecate and remove UserIdentity::getActorId()
Open, HighPublic

Description

While intuitively UserIdentity::getActorId() seems useful to have, there is no way to know at which point an ID will be assigned to an actor. Any code calling the getter would have to implement a fallback for the case no actor ID is present yet. And any code creating a UserIdentity value object would have to check if an ID has been assigned yet.

For this reason, the actor ID should be treated as an optimization/normalization internal to the storage layer, and not modeled on the level of domain entities (the "user"). Application logic should not know or care about actor IDs. Storage level code should rely on the ActorNormalization service to obtain actor IDs when needed for database operations.

  • soft-deprecate getActorId()
  • remove known callers of getActorId()
  • hard-deprecate getActorId()
  • wait for at least one release branch and three months per the stable interface policy
  • remove getActorId()

Event Timeline

daniel renamed this task from Deprecate UserIdentity::getActorId() to Deprecate and remove UserIdentity::getActorId() .Feb 8 2021, 7:35 PM
daniel triaged this task as Medium priority.
daniel created this task.
daniel updated the task description. (Show Details)
daniel raised the priority of this task from Medium to High.

Change 664325 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] EXPERIMENT: stop using getActorId

https://gerrit.wikimedia.org/r/664325

Change 665546 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Remove $actor field from UsererIdentityValue

https://gerrit.wikimedia.org/r/665546

Change 667142 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Deprecate UserIdentity::getActorId()

https://gerrit.wikimedia.org/r/667142

Change 664325 merged by jenkins-bot:
[mediawiki/core@master] ActorStore: introduce findActorIdByName

https://gerrit.wikimedia.org/r/664325

Change 671181 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/AbuseFilter@master] UserIdentityValue: do not provide actor ID

https://gerrit.wikimedia.org/r/671181

Change 667142 merged by jenkins-bot:
[mediawiki/core@master] Deprecate UserIdentity::getActorId()

https://gerrit.wikimedia.org/r/667142

Change 671181 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] UserIdentityValue: do not provide actor ID

https://gerrit.wikimedia.org/r/671181

Is there an ETA on this? We have every week ~250,000 DBPerformance log messages due to the invalidateCache() call causing a connection to the main DB in this code:

       /**
	 * Sets the actor id.
	 *
	 * This method is deprecated upon introduction. It only exists for transition to ActorStore,
	 * and will be removed shortly - T274148
	 *
	 * @internal
	 * @deprecated since 1.36
	 * @param int $actorId
	 */
	public function setActorId( int $actorId ) {
		$this->mActorId = $actorId;
		$this->invalidateCache();
		$this->setItemLoaded( 'actor' );
	}

Stacktrace:

from /srv/mediawiki/php-1.36.0-wmf.36/includes/libs/rdbms/TransactionProfiler.php(430)
#0 /srv/mediawiki/php-1.36.0-wmf.36/includes/libs/rdbms/TransactionProfiler.php(212): Wikimedia\Rdbms\TransactionProfiler->reportExpectationViolated(string, string, integer)
#1 /srv/mediawiki/php-1.36.0-wmf.36/includes/libs/rdbms/loadbalancer/LoadBalancer.php(1006): Wikimedia\Rdbms\TransactionProfiler->recordConnection(string, string, boolean)
#2 /srv/mediawiki/php-1.36.0-wmf.36/includes/libs/rdbms/loadbalancer/LoadBalancer.php(960): Wikimedia\Rdbms\LoadBalancer->getServerConnection(integer, string, integer)
#3 /srv/mediawiki/php-1.36.0-wmf.36/includes/libs/rdbms/loadbalancer/LoadBalancer.php(1099): Wikimedia\Rdbms\LoadBalancer->getConnection(integer, array, string, integer)
#4 /srv/mediawiki/php-1.36.0-wmf.36/includes/user/User.php(2360): Wikimedia\Rdbms\LoadBalancer->getConnectionRef(integer)
#5 /srv/mediawiki/php-1.36.0-wmf.36/includes/user/User.php(2376): User->clearSharedCache(string)
#6 /srv/mediawiki/php-1.36.0-wmf.36/includes/user/User.php(2200): User->invalidateCache()
#7 /srv/mediawiki/php-1.36.0-wmf.36/includes/user/ActorStore.php(320): User->setActorId(integer)
#8 /srv/mediawiki/php-1.36.0-wmf.36/includes/user/ActorStore.php(367): MediaWiki\User\ActorStore->attachActorId(User, integer)
#9 /srv/mediawiki/php-1.36.0-wmf.36/includes/ActorMigration.php(453): MediaWiki\User\ActorStore->findActorId(User, Wikimedia\Rdbms\DBConnRef)
#10 /srv/mediawiki/php-1.36.0-wmf.36/includes/user/UserEditTracker.php(155): ActorMigration->getWhere(Wikimedia\Rdbms\DBConnRef, string, array)
#11 /srv/mediawiki/php-1.36.0-wmf.36/includes/user/UserEditTracker.php(138): MediaWiki\User\UserEditTracker->getUserEditTimestamp(User, integer)
#12 /srv/mediawiki/php-1.36.0-wmf.36/extensions/GrowthExperiments/includes/HomepageModules/Mentorship.php(69): MediaWiki\User\UserEditTracker->getLatestEditTimestamp(User)
#13 /srv/mediawiki/php-1.36.0-wmf.36/extensions/GrowthExperiments/includes/HelpPanelHooks.php(195): GrowthExperiments\HomepageModules\Mentorship::getMentorLastActive(User, User, RequestContext)
#14 /srv/mediawiki/php-1.36.0-wmf.36/extensions/GrowthExperiments/includes/HelpPanelHooks.php(131): GrowthExperiments\HelpPanelHooks::getMentorData(GlobalVarConfig, User, RequestContext)
#15 /srv/mediawiki/php-1.36.0-wmf.36/includes/HookContainer/HookContainer.php(330): GrowthExperiments\HelpPanelHooks::onBeforePageDisplay(OutputPage, SkinVector)
#16 /srv/mediawiki/php-1.36.0-wmf.36/includes/HookContainer/HookContainer.php(137): MediaWiki\HookContainer\HookContainer->callLegacyHook(string, array, array, array)
#17 /srv/mediawiki/php-1.36.0-wmf.36/includes/HookContainer/HookRunner.php(1008): MediaWiki\HookContainer\HookContainer->run(string, array, array)
#18 /srv/mediawiki/php-1.36.0-wmf.36/includes/OutputPage.php(2629): MediaWiki\HookContainer\HookRunner->onBeforePageDisplay(OutputPage, SkinVector)
#19 /srv/mediawiki/php-1.36.0-wmf.36/includes/MediaWiki.php(931): OutputPage->output(boolean)
#20 /srv/mediawiki/php-1.36.0-wmf.36/includes/MediaWiki.php(944): MediaWiki::{closure}()
#21 /srv/mediawiki/php-1.36.0-wmf.36/includes/MediaWiki.php(547): MediaWiki->main()
#22 /srv/mediawiki/php-1.36.0-wmf.36/index.php(53): MediaWiki->run()
#23 /srv/mediawiki/php-1.36.0-wmf.36/index.php(46): wfIndexMain()
#24 /srv/mediawiki/w/index.php(3): require(string)
#25 {main}

Change 665546 merged by jenkins-bot:

[mediawiki/core@master] Remove $actor field from UsererIdentityValue

https://gerrit.wikimedia.org/r/665546

Change 679840 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@REL1_36] Remove $actor field from UsererIdentityValue

https://gerrit.wikimedia.org/r/679840

Change 679969 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/FileImporter@master] Remove actor arg from UserIdentityValue in FileRevisionFromRemoteUrl

https://gerrit.wikimedia.org/r/679969

Change 679969 merged by jenkins-bot:

[mediawiki/extensions/FileImporter@master] Remove actor arg from UserIdentityValue in FileRevisionFromRemoteUrl

https://gerrit.wikimedia.org/r/679969

Is there an ETA on this? We have every week ~250,000 DBPerformance log messages due to the invalidateCache() call causing a connection to the main DB in this code

@daniel & @Pchelolo could you let me know your thoughts on the above question please? If it's not going to be fixed any time soon, I can set up a filter for errors originating from this code in the Growth team dashboard.

Change 681344 had a related patch set uploaded (by Ppchelko; author: Ppchelko):

[mediawiki/core@master] User::setActorId only invalidate cache if required

https://gerrit.wikimedia.org/r/681344

Change 681344 merged by jenkins-bot:

[mediawiki/core@master] User::setActorId only invalidate cache if required

https://gerrit.wikimedia.org/r/681344

Is there an ETA on this? We have every week ~250,000 DBPerformance log messages due to the invalidateCache() call causing a connection to the main DB in this code

@daniel & @Pchelolo could you let me know your thoughts on the above question please? If it's not going to be fixed any time soon, I can set up a filter for errors originating from this code in the Growth team dashboard.

The time is now :) The patch above should in theory make this error either disappear or get to a considerably lower rates. We can backport it if it's urgent, otherwise it should show up in the next train.

Is there an ETA on this? We have every week ~250,000 DBPerformance log messages due to the invalidateCache() call causing a connection to the main DB in this code

@daniel & @Pchelolo could you let me know your thoughts on the above question please? If it's not going to be fixed any time soon, I can set up a filter for errors originating from this code in the Growth team dashboard.

The time is now :) The patch above should in theory make this error either disappear or get to a considerably lower rates. We can backport it if it's urgent, otherwise it should show up in the next train.

Thank you for that! I think it's OK to leave it for the next train.

Change 679840 merged by jenkins-bot:

[mediawiki/core@REL1_36] Remove $actor field from UsererIdentityValue

https://gerrit.wikimedia.org/r/679840

Change 689199 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/GoogleLogin@master] Remove actor arg from UserIdentityValue in FileRevisionFromRemoteUrl

https://gerrit.wikimedia.org/r/689199

Change 689199 merged by Umherirrender:

[mediawiki/extensions/GoogleLogin@master] Remove actor arg from UserIdentityValue in tests

https://gerrit.wikimedia.org/r/689199

Change 691314 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/GoogleLogin@REL1_36] Remove actor arg from UserIdentityValue in tests

https://gerrit.wikimedia.org/r/691314

Change 691314 merged by Umherirrender:

[mediawiki/extensions/GoogleLogin@REL1_36] Remove actor arg from UserIdentityValue in tests

https://gerrit.wikimedia.org/r/691314