YARN-11667. Federation: ResourceRequestComparator occurs NPE when using low version of hadoop submit application

Description of PR
When a application is submitted using a lower version of hadoop and the Resource Request built by AM has no ExecutionTypeRequest. After the Resource Request is submitted to AMRMProxy, the NPE occurs when AMRMProxy reconstructs the Allocate Request to add Resource Request to its ask
How was this patch tested?
Manual Verification and UT
For code changes:

[x] Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
[x] Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
[x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
[x] If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

:broken_heart: -1 overall

Vote
Subsystem
Runtime
Logfile
Comment

+0 :ok:
reexec
7m 30s

Docker mode activated.

_ Prechecks _

+1 :green_heart:
dupname
0m  0s

No case conflicting files found.

+0 :ok:
codespell
0m  0s

codespell was not available.

+0 :ok:
detsecrets
0m  0s

detect-secrets was not available.

+1 :green_heart:
@author
0m  0s

The patch does not contain any @author tags.

+1 :green_heart:
test4tests
0m  0s

The patch appears to include 1 new or modified test files.

_ trunk Compile Tests _

+0 :ok:
mvndep
14m 40s

Maven dependency ordering for branch

+1 :green_heart:
mvninstall
19m 57s

trunk passed

+1 :green_heart:
compile
3m 44s

trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
compile
3m 21s

trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
checkstyle
0m 58s

trunk passed

+1 :green_heart:
mvnsite
1m 10s

trunk passed

+1 :green_heart:
javadoc
1m 10s

trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javadoc
0m 59s

trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
spotbugs
2m 10s

trunk passed

+1 :green_heart:
shadedclient
19m 57s

branch has no errors when building and testing our client artifacts.

_ Patch Compile Tests _

+0 :ok:
mvndep
0m 20s

Maven dependency ordering for patch

+1 :green_heart:
mvninstall
0m 36s

the patch passed

+1 :green_heart:
compile
3m 27s

the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javac
3m 27s

the patch passed

+1 :green_heart:
compile
3m 24s

the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
javac
3m 24s

the patch passed

+1 :green_heart:
blanks
0m  0s

The patch has no blanks issues.

-0 :warning:
checkstyle
0m 55s
/results-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
hadoop-yarn-project/hadoop-yarn: The patch generated 1 new + 14 unchanged - 0 fixed = 15 total (was 14)

+1 :green_heart:
mvnsite
1m  0s

the patch passed

+1 :green_heart:
javadoc
0m 55s

the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javadoc
0m 56s

the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
spotbugs
2m 10s

the patch passed

+1 :green_heart:
shadedclient
19m 54s

patch has no errors when building and testing our client artifacts.

_ Other Tests _

+1 :green_heart:
unit
0m 47s

hadoop-yarn-api in the patch passed.

-1 :x:
unit
25m 34s
/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-client.txt
hadoop-yarn-client in the patch passed.

+1 :green_heart:
asflicense
0m 40s

The patch does not generate ASF License warnings.

140m 38s

Reason
Tests

Failed junit tests
hadoop.yarn.client.TestYarnApiClasses

Subsystem
Report/Notes

Docker
ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/1/artifact/out/Dockerfile

GITHUB PR
https://github.com/apache/hadoop/pull/6648

Optional Tests
dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets

uname
Linux 56635d28f057 5.15.0-94-generic #104-Ubuntu SMP Tue Jan 9 15:25:40 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Build tool
maven

Personality
dev-support/bin/hadoop.sh

git revision
trunk / 54bb13a04b6f2717a828adc97f94eef6797c185b

Default Java
Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

Multi-JDK versions
/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

Test Results
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/1/testReport/

Max. process+thread count
578 (vs. ulimit of 5500)

modules
C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client U: hadoop-yarn-project/hadoop-yarn

Console output
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/1/console

versions
git=2.25.1 maven=3.6.3 spotbugs=4.2.2

Powered by
Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

:confetti_ball: +1 overall

Vote
Subsystem
Runtime
Logfile
Comment

+0 :ok:
reexec
0m 21s

Docker mode activated.

_ Prechecks _

+1 :green_heart:
dupname
0m  0s

No case conflicting files found.

+0 :ok:
codespell
0m  0s

codespell was not available.

+0 :ok:
detsecrets
0m  0s

detect-secrets was not available.

+1 :green_heart:
@author
0m  0s

The patch does not contain any @author tags.

+1 :green_heart:
test4tests
0m  0s

The patch appears to include 1 new or modified test files.

_ trunk Compile Tests _

+0 :ok:
mvndep
13m 45s

Maven dependency ordering for branch

+1 :green_heart:
mvninstall
19m 52s

trunk passed

+1 :green_heart:
compile
3m 40s

trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
compile
3m 28s

trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
checkstyle
1m  2s

trunk passed

+1 :green_heart:
mvnsite
1m 12s

trunk passed

+1 :green_heart:
javadoc
1m  7s

trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javadoc
1m  5s

trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
spotbugs
2m  2s

trunk passed

+1 :green_heart:
shadedclient
20m 57s

branch has no errors when building and testing our client artifacts.

_ Patch Compile Tests _

+0 :ok:
mvndep
0m 23s

Maven dependency ordering for patch

+1 :green_heart:
mvninstall
0m 37s

the patch passed

+1 :green_heart:
compile
3m 35s

the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javac
3m 35s

the patch passed

+1 :green_heart:
compile
3m 32s

the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
javac
3m 32s

the patch passed

+1 :green_heart:
blanks
0m  0s

The patch has no blanks issues.

-0 :warning:
checkstyle
0m 57s
/results-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
hadoop-yarn-project/hadoop-yarn: The patch generated 1 new + 14 unchanged - 0 fixed = 15 total (was 14)

+1 :green_heart:
mvnsite
1m  1s

the patch passed

+1 :green_heart:
javadoc
0m 52s

the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javadoc
0m 52s

the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
spotbugs
2m  0s

the patch passed

+1 :green_heart:
shadedclient
21m 30s

patch has no errors when building and testing our client artifacts.

_ Other Tests _

+1 :green_heart:
unit
0m 42s

hadoop-yarn-api in the patch passed.

+1 :green_heart:
unit
24m 50s

hadoop-yarn-client in the patch passed.

+1 :green_heart:
asflicense
0m 32s

The patch does not generate ASF License warnings.

134m 20s

Subsystem
Report/Notes

Docker
ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/2/artifact/out/Dockerfile

GITHUB PR
https://github.com/apache/hadoop/pull/6648

Optional Tests
dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets

uname
Linux d6c3acd693a2 5.15.0-94-generic #104-Ubuntu SMP Tue Jan 9 15:25:40 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Build tool
maven

Personality
dev-support/bin/hadoop.sh

git revision
trunk / 42e05bb0e8c7db7f62445feeb685f08e934d49ac

Default Java
Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

Multi-JDK versions
/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

Test Results
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/2/testReport/

Max. process+thread count
687 (vs. ulimit of 5500)

modules
C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client U: hadoop-yarn-project/hadoop-yarn

Console output
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/2/console

versions
git=2.25.1 maven=3.6.3 spotbugs=4.2.2

Powered by
Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

:confetti_ball: +1 overall

Vote
Subsystem
Runtime
Logfile
Comment

+0 :ok:
reexec
0m 36s

Docker mode activated.

_ Prechecks _

+1 :green_heart:
dupname
0m  0s

No case conflicting files found.

+0 :ok:
codespell
0m  1s

codespell was not available.

+0 :ok:
detsecrets
0m  1s

detect-secrets was not available.

+1 :green_heart:
@author
0m  0s

The patch does not contain any @author tags.

+1 :green_heart:
test4tests
0m  0s

The patch appears to include 1 new or modified test files.

_ trunk Compile Tests _

+0 :ok:
mvndep
14m  1s

Maven dependency ordering for branch

+1 :green_heart:
mvninstall
36m 50s

trunk passed

+1 :green_heart:
compile
8m  8s

trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
compile
7m 15s

trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
checkstyle
1m 54s

trunk passed

+1 :green_heart:
mvnsite
1m 40s

trunk passed

+1 :green_heart:
javadoc
1m 33s

trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javadoc
1m 23s

trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
spotbugs
3m 23s

trunk passed

+1 :green_heart:
shadedclient
37m  2s

branch has no errors when building and testing our client artifacts.

_ Patch Compile Tests _

+0 :ok:
mvndep
0m 31s

Maven dependency ordering for patch

+1 :green_heart:
mvninstall
1m  3s

the patch passed

+1 :green_heart:
compile
7m 15s

the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javac
7m 15s

the patch passed

+1 :green_heart:
compile
7m  6s

the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
javac
7m  6s

the patch passed

+1 :green_heart:
blanks
0m  0s

The patch has no blanks issues.

-0 :warning:
checkstyle
1m 49s
/results-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
hadoop-yarn-project/hadoop-yarn: The patch generated 1 new + 14 unchanged - 0 fixed = 15 total (was 14)

+1 :green_heart:
mvnsite
1m 31s

the patch passed

+1 :green_heart:
javadoc
1m 20s

the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javadoc
1m 18s

the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
spotbugs
3m 38s

the patch passed

+1 :green_heart:
shadedclient
35m  6s

patch has no errors when building and testing our client artifacts.

_ Other Tests _

+1 :green_heart:
unit
1m  7s

hadoop-yarn-api in the patch passed.

+1 :green_heart:
unit
28m 53s

hadoop-yarn-client in the patch passed.

+1 :green_heart:
asflicense
1m  4s

The patch does not generate ASF License warnings.

212m 21s

Subsystem
Report/Notes

Docker
ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/3/artifact/out/Dockerfile

GITHUB PR
https://github.com/apache/hadoop/pull/6648

Optional Tests
dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets

uname
Linux f5faf9fb4f4d 5.15.0-94-generic #104-Ubuntu SMP Tue Jan 9 15:25:40 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Build tool
maven

Personality
dev-support/bin/hadoop.sh

git revision
trunk / a85f9f54c1e6bed41227a2f48587e2b42d821c0a

Default Java
Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

Multi-JDK versions
/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

Test Results
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/3/testReport/

Max. process+thread count
728 (vs. ulimit of 5500)

modules
C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client U: hadoop-yarn-project/hadoop-yarn

Console output
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/3/console

versions
git=2.25.1 maven=3.6.3 spotbugs=4.2.2

Powered by
Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

@qiuliang988 Thank you for your contribution! but I'm sorry, I do not agree with this change. We cannot always maintain backward compatibility, and the best approach would be to find a way to upgrade the version.

:confetti_ball: +1 overall

Vote
Subsystem
Runtime
Logfile
Comment

+0 :ok:
reexec
0m 30s

Docker mode activated.

_ Prechecks _

+1 :green_heart:
dupname
0m  1s

No case conflicting files found.

+0 :ok:
codespell
0m  0s

codespell was not available.

+0 :ok:
detsecrets
0m  0s

detect-secrets was not available.

+1 :green_heart:
@author
0m  0s

The patch does not contain any @author tags.

+1 :green_heart:
test4tests
0m  0s

The patch appears to include 1 new or modified test files.

_ trunk Compile Tests _

+0 :ok:
mvndep
14m  0s

Maven dependency ordering for branch

+1 :green_heart:
mvninstall
31m 10s

trunk passed

+1 :green_heart:
compile
7m 36s

trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
compile
6m 57s

trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
checkstyle
1m 57s

trunk passed

+1 :green_heart:
mvnsite
1m 52s

trunk passed

+1 :green_heart:
javadoc
1m 45s

trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javadoc
1m 35s

trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
spotbugs
3m 24s

trunk passed

+1 :green_heart:
shadedclient
33m 38s

branch has no errors when building and testing our client artifacts.

_ Patch Compile Tests _

+0 :ok:
mvndep
0m 32s

Maven dependency ordering for patch

+1 :green_heart:
mvninstall
1m  3s

the patch passed

+1 :green_heart:
compile
6m 50s

the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javac
6m 50s

the patch passed

+1 :green_heart:
compile
6m 58s

the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
javac
6m 58s

the patch passed

+1 :green_heart:
blanks
0m  0s

The patch has no blanks issues.

+1 :green_heart:
checkstyle
1m 47s

the patch passed

+1 :green_heart:
mvnsite
1m 37s

the patch passed

+1 :green_heart:
javadoc
1m 31s

the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javadoc
1m 25s

the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
spotbugs
3m 33s

the patch passed

+1 :green_heart:
shadedclient
33m 56s

patch has no errors when building and testing our client artifacts.

_ Other Tests _

+1 :green_heart:
unit
1m 11s

hadoop-yarn-api in the patch passed.

+1 :green_heart:
unit
29m  7s

hadoop-yarn-client in the patch passed.

+1 :green_heart:
asflicense
1m  4s

The patch does not generate ASF License warnings.

201m 44s

Subsystem
Report/Notes

Docker
ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/4/artifact/out/Dockerfile

GITHUB PR
https://github.com/apache/hadoop/pull/6648

Optional Tests
dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets

uname
Linux 1de32bf9fbba 5.15.0-94-generic #104-Ubuntu SMP Tue Jan 9 15:25:40 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Build tool
maven

Personality
dev-support/bin/hadoop.sh

git revision
trunk / 4be56d2c6590fe652f15a0d896645d9f0ef62982

Default Java
Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

Multi-JDK versions
/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

Test Results
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/4/testReport/

Max. process+thread count
661 (vs. ulimit of 5500)

modules
C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client U: hadoop-yarn-project/hadoop-yarn

Console output
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/4/console

versions
git=2.25.1 maven=3.6.3 spotbugs=4.2.2

Powered by
Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

@qiuliang988 Thank you for your contribution! but I'm sorry, I do not agree with this change. We cannot always maintain backward compatibility, and the best approach would be to find a way to upgrade the version.

@slfan1989 Thanks for your advice! I will close this PR

:confetti_ball: +1 overall

Vote
Subsystem
Runtime
Logfile
Comment

+0 :ok:
reexec
17m 19s

Docker mode activated.

_ Prechecks _

+1 :green_heart:
dupname
0m  0s

No case conflicting files found.

+0 :ok:
codespell
0m  0s

codespell was not available.

+0 :ok:
detsecrets
0m  0s

detect-secrets was not available.

+1 :green_heart:
@author
0m  0s

The patch does not contain any @author tags.

+1 :green_heart:
test4tests
0m  0s

The patch appears to include 1 new or modified test files.

_ trunk Compile Tests _

+0 :ok:
mvndep
13m 51s

Maven dependency ordering for branch

+1 :green_heart:
mvninstall
36m 33s

trunk passed

+1 :green_heart:
compile
8m 19s

trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
compile
7m 31s

trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
checkstyle
2m  0s

trunk passed

+1 :green_heart:
mvnsite
1m 46s

trunk passed

+1 :green_heart:
javadoc
1m 42s

trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javadoc
1m 31s

trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
spotbugs
3m 19s

trunk passed

+1 :green_heart:
shadedclient
39m 17s

branch has no errors when building and testing our client artifacts.

_ Patch Compile Tests _

+0 :ok:
mvndep
0m 31s

Maven dependency ordering for patch

+1 :green_heart:
mvninstall
1m  2s

the patch passed

+1 :green_heart:
compile
7m 29s

the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javac
7m 29s

the patch passed

+1 :green_heart:
compile
7m 24s

the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
javac
7m 24s

the patch passed

+1 :green_heart:
blanks
0m  0s

The patch has no blanks issues.

+1 :green_heart:
checkstyle
1m 52s

the patch passed

+1 :green_heart:
mvnsite
1m 34s

the patch passed

+1 :green_heart:
javadoc
1m 28s

the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1

+1 :green_heart:
javadoc
1m 21s

the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

+1 :green_heart:
spotbugs
3m 31s

the patch passed

+1 :green_heart:
shadedclient
39m 27s

patch has no errors when building and testing our client artifacts.

_ Other Tests _

+1 :green_heart:
unit
1m  9s

hadoop-yarn-api in the patch passed.

+1 :green_heart:
unit
28m 39s

hadoop-yarn-client in the patch passed.

+1 :green_heart:
asflicense
1m  1s

The patch does not generate ASF License warnings.

236m 38s

Subsystem
Report/Notes

Docker
ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/5/artifact/out/Dockerfile

GITHUB PR
https://github.com/apache/hadoop/pull/6648

Optional Tests
dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets

uname
Linux 08f70c235076 5.15.0-94-generic #104-Ubuntu SMP Tue Jan 9 15:25:40 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Build tool
maven

Personality
dev-support/bin/hadoop.sh

git revision
trunk / 4be56d2c6590fe652f15a0d896645d9f0ef62982

Default Java
Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

Multi-JDK versions
/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06

Test Results
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/5/testReport/

Max. process+thread count
586 (vs. ulimit of 5500)

modules
C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client U: hadoop-yarn-project/hadoop-yarn

Console output
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6648/5/console

versions
git=2.25.1 maven=3.6.3 spotbugs=4.2.2

Powered by
Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.


Improvement in our achievement page

The card overlaps on show more button.
Before

After card opens

Already an issue exaists for this


Revert "Start markdown"

Reverts Exp-Communicate-Using-Markdown-Cohort-1/series-communicate-using-markdown-aliyabatoolalvi#3

salam
how are you


Add description to Display/View combination

To be consistent with other objects a description field was added to a display/view combination.  The description field could be used in an UI for pop up help.
It follows in the style of the current API with a new method getDisplayDescription(const char *display, const char *view) method added to the Config object.  A second overloaded addDisplay method was added with a description parameter instead of making a description parameter with a default value.  This should ensure binary compatibility with the current library.

Do we really need another Config::addDisplay I think just break the ABI and add the description argument to the existing one.

Closing this as it is no longer relevant after the merge of PR #1038, which added the same functionality.


[SDK 48] Failed to run gradlew (Android) and podfile (iOS)

Build/Submit details page URL
https://expo.dev/accounts/homeguardian/projects/homeguardian-ai/builds/f3d1a32e-84dd-4cd3-812c-38fe2c18015f
Summary
I'm having a Expo app (SDK 48) within a mono-repo and trying to deploy it as builds for both Android and iOS by using command eas build -p all. However, it fails to build at the step Run gradlew and Install pods on Android and iOS respectively.

Managed or bare?
Bare
Environment
Output from npx expo-env-info:
expo-env-info 1.0.5 environment info:
    System:
      OS: macOS 13.4
      Shell: 5.9 - /bin/zsh
    Binaries:
      Node: 18.16.0 - ~/.nvm/versions/node/v18.16.0/bin/node
      Yarn: 1.22.19 - /opt/homebrew/bin/yarn
      npm: 9.5.1 - ~/.nvm/versions/node/v18.16.0/bin/npm
    SDKs:
      iOS SDK:
        Platforms: DriverKit 22.4, iOS 16.4, macOS 13.3, tvOS 16.4, watchOS 9.4
    IDEs:
      Android Studio: 2022.2 AI-222.4459.24.2221.9862592
      Xcode: 14.3/14E222b - /usr/bin/xcodebuild
    npmPackages:
      react: 18.2.0 => 18.2.0
      react-native: 0.71.8 => 0.71.8
    npmGlobalPackages:
      expo-cli: 6.3.7
    Expo Workflow: bare

Output from npx expo-doctor:
✔ Validating global prerequisites versions passed
✔ Checking for incompatible packages passed
✔ Checking for conflicting global packages in project passed
✔ Verifying prebuild support package versions are compatible passed
✔ Checking dependency versions for compatibility with the installed Expo SDK passed
✔ Validating Expo Config passed
✔ Checking package.json for common issues passed

Error output
No response
Reproducible demo or steps to reproduce from a blank project
To reproduce it, simply create an Expo app within the mono-repo /apps directory (I am currently using Turborepo).
create-expo-app my-expo-app
Mono-repo package.json excerpt:
{
  "name": "turbo",
  "version": "0.1.0",
  "private": true,
  "packageManager"<EMAIL_ADDRESS>  "workspaces": {
    "packages": [
      "apps/**/*",
      "packages/**/*"
    ],
    "nohoist": [
      "**/react-native",
      "**/react-native/**"
    ]
  },
[...]
}

apps/my-expo-apps/eas.json:
{
  "cli": {
    "version": ">= 3.13.2"
  },
  "build": {
    "development": {
      "developmentClient": true,
      "distribution": "internal"
    },
    "preview": {
      "distribution": "internal"
    },
    "production": {}
  },
  "submit": {
    "production": {}
  }
}

In apps/my-expo-app/metro.config.js, I added this since Expo recommended to append this for SDK 48:
config.transformer.minifierPath = "metro-minify-terser"
config.transformer.minifierConfig = {
  // Terser options...
}

I also run prebuild npx expo prebuild to generate Podfile and build.grandle; however, with or without prebuilding the eas build still returns the same error logs.

Update:
I just downgraded to Expo 47 and it worked.
As of now I assume 48 is too unstable for build version.
Hopefully it'll be fixed soon.


feat: added verbose logging to errors

Please describe the changes this PR makes and why it should be merged:
Status

[x] Code changes have been tested.

I was unable to run the tests locally. It seems like they're commented out in CI, so I'm unsure how to verify the behavior I created. Please advise!! (edit: I spoke with @zaida04 about this and he's going to look into this)

Semantic versioning classification:

[x] This PR changes the library's interface (methods or parameters added)
[x] This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
[ ] This PR only includes non-code changes, like changes to documentation, README, etc.

Yo! Ryan from Guilded Staff here! :pizza:
First of all, thank you so much for your continued interest in Guilded and its bot API!!! We love seeing the passion in the community for building fun stuff against the product. This library has opened the door to JavaScript and TypeScript devs to be able to write bots quickly in these languages. Nice work there!!
This PR serves to bring this library to conformance with the "Verbose Logging" guildeline in Guilded's Bot Library Guidelines document. We have been having trouble working through various support issues recently in our community server for this library, and I think a lot of the pain points come from missing some details from the "Verbose Logging" [1]. From the brief look I took, it does look like this library conforms to the other guidelines in that doc though, so nice work otherwise!!
Some outstanding issues with this PR that I will hand off to the library maintainers to fix (mostly done in the interest of time, sorry for wall of text!):

There is a lot of wonkiness in the error constructor because I was trying to appease backwards compatibility of the exported errors. I am not a TypeScript expert, so it's entirely possible this can be done in a better way [2]. I'd encourage you to heavily scrutinize what I wrote to ensure I didn't leave anything through the cracks. I'd also encourage you to create a deprecation path for the previous approach so that this wonkiness can be eventually removed if not done during this code review.
I did not obfuscate the authorization header due because I don't know how this library handles case sensitivity of the header given the underlying object is a POJO, not a Headers object. This must be done before merging, as it's a security risk outputting it as-is. There are loud FIXMEs explaining where
I did not spend time properly typing everything. I'll leave that up to the library devs though because there may be context I'm missing as to how to properly type things in accordance to patterns that exist in the rest of the library. I'd encourage you to fill the gaps that I left (e.g., not using any, etc.)!
I am not sure how best to document this new behavior, as the constructor may be more of an internally used mechanism and the errors are only exported for instanceof checks. It's possible nothing is needed here?
There may be miscellaneous other things that don't conform to patterns that exist elsewhere in the codebase. Please feel free to blow away and rework anything I did here as long as the end state is that the requests and responses are being output as I've originally done here.

I encourage you to prioritize fixing the outstanding issues, merging the PR, and creating a new release so that the Guilded support team can better help debugging various API issues going forward. Please let me know how I can help in facilitating a quick turnaround here.

[1] It's entirely possible that the existing verbiage in our "Verbose Logging" guideline is not clear enough for what libraries should look like. Please let us know if that is the case so that we can ensure other bot/library devs can understand this critical point!
[2] A question I had: can you type entire function arg arrays so that the V1 and V2 ways can be mutually exclusive? This would potentially make it so the compiler enforces that the constructors can only be invoked in 2 specific ways: the v2 way in (msg: string, methodOrRequest: RequestOptions, pathOrResponse: ResponseDetails) or v1 way in (msg: string, methodOrRequest: string, pathOrResponse: string, code: number). This is in contrast with what is currently written that technically allows for mixing and matching of parameters such that you could invoke it like (msg: string, methodOrRequest: string, pathOrResponse: ResponseDetails), which is somewhere in between V1 and V2 usage and is bad.

Thank you for your contribution fellow pizza man! 🍕

Hey! Thank you for the contribution, we really appreciate it :D
I did some changes on your PR for formatting/type narrowing to bring it up to merge standard. It is pending testing with our examples repo to ensure it stays consistent in behavior.
I have also done the following:

Added obfuscation on the auth header for error objects
Removed backwards compatibility for the old way of instantiating GuildedAPIError and PermissionsError in favor of having @guildedjs/rest release on a semver major increment.

In regards to [2] in your footnotes: if we wanted to keep the same functionality but have different signatures so that there isn't incompatible type mix and matching, we could use method overloaders to enforce different, exclusive signatures.
Don't worry about any documentation regarding your changes either, as our TSDoc generator automatically creates new docs files for parsing/hosting on every commit/merge to main.
And a response to [1] as well, the guidelines are clear enough for us to understand. We had actually attempted in the past to conform to the logging standard set in that document, but ran into an issue as we wanted the actual Response object to also be passed as an object whole into an event on the .emitter property of the RestManager so that users could have opt-in logging on any REST request, regardless of failure or success. However, we found out it wasn't doable without consuming/cloning the Response object which was not advisable due to thread lockups. As such, we abandoned the attempt and settled on trying again later at some point before the publicization of the API (which we didn't hold ourselves to).

oooh the method overloaders feature of TS is interesting. i think this would've been what i would've wanted had we kept the wonkiness i wrote :).  thanks for the tip!

(For record keeping sake, I'll note in this PR that screenshots of the error outputs were sent privately)

Thank you for the contribution! 🚀


Fixing a double dot typo in one plugin filename

@koenbok can you please merge it?
I'd like to contribute a .gitignore file to the project skeleton.

Thanks, @kartoch!
Cheers,


[BUG] Set-PnPListItem not working on lists with more than 12 lookup columns

Expected behavior
Using Set-PnPListItem to update some values of a list item in a list having more than 12 lookup columns shouldn't run into this lookup column threshold issue as long as we are not trying to set more than 12 lookup columns in that operation.
Actual behavior
We will get the error message that the list we are operating on is exeeding the 12 lookup column threshold and thus the query couln't be processed.
Steps to reproduce behavior

Create a custom list and add more than 12 lookup columns in total (Person/Group, Taxonomy or actual SP list lookups - no matter what)
Try to update a list item in that list - just setting e. g. one of the taxonomy fields.

What is the version of the Cmdlet module you are running?
2.12.0
Which operating system/environment are you running PnP PowerShell on?

[ x] Windows
[ ] Linux
[ ] MacOS
[ ] Azure Cloud Shell
[ ] Azure Functions
[ ] Other : please specify

I've checked the code to clarify why PnP.PowerShell is obviouslty trying to query all list columns while I'm just trying to update e.g just one field.
Found the following code in class ListItemHelper:

I think it should be possible to query just the fields that are acutally rerference in the Cmdlets "value" Parameter. That way it should be easy to avoid that threshold issue.
I've verified my theory using the Set-PnPTaxonomyFieldValue Cmdlet on that same list - with no issues at all. Here the code (in Microsoft.SharePointClient.TaxonomyExtensions)  is just loading the field in question and everything works fine:

By the way - this is the error message we're currently facing:
The query cannot be completed because the number of lookup columns it contains exceeds the lookup column threshold.
I'm even getting this error, if I'm just trying to change the item's content type using Set-PnPListItem. No additional values involved. And all this just because Set-FieldValues is trying to load all fields involved (for whatever reason).

As a workaround, what happens if you load the item and updated it manually?
Something like this (don't have PowerShell handy right now, but you should be able to get it to work)
$li = Get-PnPListItem -Identity 1
$li["Fieldname"] = "New Value"
$li.update();
Invoke-PnPQuery

@Tanddant  Yes, indeed. That's working for me. I've already tested a fix for the code behind Set-PnPListItem. The trick is to avoid the query for all fields as mentioned in my post above:

I'm avoiding to load all fields by query. The only fields the code now loads are the ones involved, if you provide any value updates.
My fix so far just involved the single item update - not the batch operation. The latter one is unfortunately far more tricky.


Can't forward RPC URL

export ETH_RPC_URL="https://mainnet.infura.io/v3/..."
./ddapp.sh seth call 0x6b175474e89094c44da98b954eedeac495271d0f "balanceOf(address)(uint)" 0x40ec5b33f54e0e8a33a975908c5ba1c14e5bbbdf
/nix/store/kxj6cblcsd1qcbbxlmbswwrn89zcmgd6-bash-4.4-p23/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
curl: (3) bad range specification in URL position 1835:
https://mainnet.infura.io/v3/...
SESSION=...
SHELL=/bin/zsh
TMPDIR=/var/folders/km/...

If enter a bash shell in container it works:
./ddapp.sh bash
root@de1ddcf8f454:/dapptools# export ETH_RPC_URL="https://mainnet.infura.io/v3/..."
root@de1ddcf8f454:/dapptools# seth call 0x6b175474e89094c44da98b954eedeac495271d0f "balanceOf(address)(uint)" 0x40ec5b33f54e0e8a33a975908c5ba1c14e5bbbdf
/nix/store/kxj6cblcsd1qcbbxlmbswwrn89zcmgd6-bash-4.4-p23/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
294119505230795042555348963

I have a feeling that has to do with how docker parses the string. It reads the ENV variable but for some reason, it's not serialized correctly.
Can you share the URL form at least?


JWT access token expiration only checked once

Once the client is authenticated, it never checks the JWT token expiration again. Thus if we use org.springframework.cloud.security.oauth2.proxy.OAuth2TokenRelayFilter, we can send out expired tokens to the resource servers, which cannot refresh it.
I think the correct behaviour should be to check expiration of JWT token, and refresh or reissue it if it is expired.

@dsyer Can you confirm that would be the correct behaviour?

A client isn't necessarily going to be able to do that pre-emptively (the token is opaque), but it can respond to 403s from the resource server and retry. that's what OAuth2RestTemplate does.


Automatically configure Postgres and MySQL databases on startup.

What was changed

Automatically create databases and apply the schemas to them for Postgres and MySQL.
Why?

In order to be able to create new environments via Terraform without having to manually create the databases between the provisioning of the cluster and the deployment of the helm chart.
Checklist

How was this tested:

I used this to provision a Postgres cluster, but no other testing has been done.  In particular, MySQL hasn't been tested at all.

Any docs updates needed?

Probably?  I'm just giving this back to hopefully help other people, but I don't have the time to do any more work on it myself.

 Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

Hi team, seems like duplicate of #275. Would love to see one of them merged

Added this patch to our branch, if anyone needs it:
https://github.com/contra/helm-charts/commit/ff0d75f302eb69106468a4d0f6f9bc2a006128a8

I can confirm this works. Can this get merged?

@rogersd I think you need to sign the Contributor License Agreement https://cla-assistant.io/temporalio/helm-charts?pullRequest=281

This PR will close this issue
https://github.com/temporalio/helm-charts/issues/349

@rogersd I think you need to sign the Contributor License Agreement https://cla-assistant.io/temporalio/helm-charts?pullRequest=281

I’ve signed it, but I’m not sure if there’s something that needs to be done for GitHub to pick that up?

@jeenadeepak What needs to happen to pass the cla?

Anyway we can help with this being approved?

I'm taking over this PR.

I'm taking over this PR.

Thanks, @mindaugasrukas !

I'm closing this in favor of #405
I had to fix commands, but overall everything is ok.


[Test] Kirkstone 1.4.33 LTS

Currently blocked on the issue with meta-rust on this issue. https://github.com/meta-rust/meta-rust/issues/450
as Kirkstone 1.4 LTS for IoTEdge is going out of support on Nov 2024. It is likely that we will not release recipes for Kirkstone for 1.4.x recipes.


Issue 124 better config (resolves #124, resolves #217, resolves #221)

This addresses issues: #221 #124 #217

I think the tests are stuck in a loop, what do we do in this case?

Branch name: check. Issue title: nope. Issue number needs to mentioned as "(resolves #124)".
Re hang: I created a new slave image and since then the agent hangs on launch.
https://github.com/BD2KGenomics/cgcloud/issues/43
For workaround, see above issue.

Tests stuck in infinite loop:
Mesos executor stderr:
DEBUG:toil.batchSystems.mesos.executor:Starting executor
I0817 17:06:20.035890  4045 exec.cpp:132] Version: 0.23.0
I0817 17:06:20.038673  4048 exec.cpp:206] Executor registered on slave 20150817-170619-16777343-5050-4020-S0
INFO:toil.batchSystems.mesos.executor:Registered with framework
DEBUG:toil.batchSystems.mesos.executor:Running task 0
DEBUG:toil.batchSystems.mesos.executor:Sending status update ...
DEBUG:toil.batchSystems.mesos.executor:sent stats message
DEBUG:toil.batchSystems.mesos.executor:Sent status update
DEBUG:toil.batchSystems.mesos.executor:Invoking command: '/mnt/ephemeral/workspace/toil/venv/bin/python2.7 -E /mnt/ephemeral/workspace/toil/src/toil/worker.py ./toil 7/f/jobjIomht'
Traceback (most recent call last):
  File "/mnt/ephemeral/workspace/toil/src/toil/worker.py", line 434, in <module>
    main()
  File "/mnt/ephemeral/workspace/toil/src/toil/worker.py", line 125, in main
    jobStore = loadJobStore(jobStoreString)
  File "/mnt/ephemeral/workspace/toil/src/toil/common.py", line 376, in loadJobStore
    return FileJobStore( jobStoreArgs, config=config )
  File "/mnt/ephemeral/workspace/toil/src/toil/jobStores/fileJobStore.py", line 35, in __init__
    self._checkJobStoreCreation(config != None, os.path.exists(self.jobStoreDir), self.jobStoreDir)
  File "/mnt/ephemeral/workspace/toil/src/toil/jobStores/abstractJobStore.py", line 80, in _checkJobStoreCreation
    "is nothing to restart." % jobStoreString)
toil.jobStores.abstractJobStore.JobStoreCreationException: The job store '/tmp/mesos/slaves/20150817-170619-16777343-5050-4020-S0/frameworks/20150817-170619-16777343-5050-4020-0000/executors/badExecutor/
runs/aced20a7-4aef-4c57-b458-0575c2f167d8/toil' does not exist, so there is nothing to restart.
DEBUG:toil.batchSystems.mesos.executor:Sending status update ...
DEBUG:toil.batchSystems.mesos.executor:Sent status update

Jenkins, test this please.

Jenkins, test this please.

The Jenkins build skipped all AWS-related tests.
b3847affbdf221a2189c2c28b4b1ff8f41a49c80 needs to be merged from master in order to fix that.

The mesos problem is caused by a relative path to the toil directory being passed with the worker (Python -E worker.py ./toil  etc...) The executor does not operate in the same directory as the master, so ./toil does not exist in it's directory. This issue came up before and was/is fixed on master by making this /.toil into an absolute path.

I pull in the master this morning?

Okay, I know what I did wrong, CJ helped me, it's running now

Okay, the only test that is reported to have failed is pytest, and I think that's a CI issue, hannes?

Also, looking at the test report it seems not all tests are being run? For example, toil.test.utils.utilTest

Jenkins, test this please.

ENOENT: [No such file or directory]: getcwd()
happens when the current directory has been deleted. The issue is specific to this branch, not CI in general since master is building ok, also with py.test.

Look like we are finally all good!


Update styled components example

Updates the styled-components example to pass HOC to renderPage() and fix SSR.
Based of the discussions at #2831

Lgtm
@mxstbr the website is already using something practically equivalent :)

Thanks guys!


Skip getRelevantEffects for invalid callbackTypes

Tests pass if the hardcoded CALLBACK_TYPES are used, and if I load data manually I think I get around the right counts of callbacks, but pretty much all the tests fail because they're not loading data (and do we want them to? seems like it would tank test performance?).
Figured I'd create a WIP PR for feedback:

b531250: the sim/battle.js logic is what I think will be the end state for this branch. It definitely adds complexity to an already complex method, but given how important this method is with respect to performance it seems it would be justified (I'm anticipating on the order of a further 20% reduction). I think we might also want to consider breaking getRelevantEffectInner up, something like 5fbae3e seems like it would make things more readable compared to HEAD?
75f629a: sim/dex.js needs work. I'm not sure about a lot of things:

is HAS_CALLBACKS comprehensive/complete?
how should inheriting and callbackTypes work? Given that it's always OK to make callbackTypes larger than expected (we just get less performance benefit) I erred on the side of caution and just copied everything from the parent.
is looking for startsWith('on') actually the most robust way to do this? Thats the heuristic I used when going over dev-tools/globals.ts to come up with the hardcoded CALLBACK_TYPES, but I'm not sure if that's right. Also, that nested for-loop is ugh.
how do I deal with the loading/test issue in an acceptable way? The naive solution for speedy tests would be could use the hardcoded set of CALLBACK_TYPES during testing and the loaded callbackTypes in prod, but then we're not actually testing reality so there's cracks for bugs to slip in.

This pull request introduces 1 alert when merging 75f629ad2dfc35c82e6749284327947fe414931c into fd0059f39b8740fd9ae5810da8097ee84cd2c05c - view on LGTM.com
new alerts:

1 for Unused variable, import, function or class

Comment posted by LGTM.com

I think it makes sense to break up getRelevantEffectInner, but is the dispatch function really necessary?

I think it makes sense to break up getRelevantEffectInner, but is the dispatch function really necessary?

I didn't think it was either, but I don't know how to constrain the type of foeThing in the recursive calls of getRelevantPokemonEffects.
Should I create a PR for 5fbae3e to discuss it further?

I'm investigating. foeThing should be Pokemon | null, and as far as I can tell, getAll is entirely unused.

Oh, wait, getAll is used for residual effects, where it's seeking 'duration'

Oops, yeah, I remembered discovering getAll can only be 'duration' as well. If we can guarantee that constraint we can further optimize (though once again, its so rarely used it doesn't really matter if it causes a bail out)

Okay, this function hasn't been cleaned up since 2011, it's just gradually gained more cruft. I'm going to clean it up, and I'm really sorry for the amount of merge conflicts you'll need to deal with after my cleanup.

I think you'll find the new implementation massively more readable, though:
https://github.com/Zarel/Pokemon-Showdown/blob/c6d8de14138976621918340885ed11b41a7ba5e2/sim/battle.js#L956-L1116

Massively more readable for sure! I'll look to redo the new branch. Did you have any thoughts about some of my other questions I posed? I think regarding test speed one answer could be to use a hardcoded constant for tests, but then add a new test to verify the constant is accurate and reflects reality.

Some more investigation - I tried to look into 'where do effects come from?':
I'm (perhaps naively?) assuming they're all hardcoded, and aren't dynamically created on the fly, so everything should be under data or data/mods. Abilities and Items are mostly straightfoward, but a bunch of things are in data/statuses (Statuses, Weather, some VolatileStatuses and SideConditions, and even Pokemon) - the problem here is that they don't all have a nice effectType which will be required for what I described in https://github.com/Zarel/Pokemon-Showdown/pull/5191#discussion_r258789987, so I think that the first step is probably going and adding that information to the data.
The other challenge is that other effects (like Terrain) are actually hidden away in nested effect fields of the EffectData type - so in order to gather them all up it'll require recursively traversing any Effect to its child effect. I don't imagine we chain these that deep, but it just makes this that much more fun :). These will also want to be given an effectType (even though the nested ones technically aren't required to have it because they're only Partial<EffectData>!)

if I load data manually I think I get around the right counts of callbacks, but pretty much all the tests fail because they're not loading data (and do we want them to? seems like it would tank test performance?)

I don't understand. What makes tests fail because we're not loading data?

is HAS_CALLBACKS comprehensive/complete?

I would use 'listener' rather than 'callback'.
You have Formats in there twice. :p Also, no. Pure effects also have listeners. And I think formats can attach more listeners using onEvent, which I'm not entirely sure why it exists considering formats can just have listeners directly.
When I say "pure effects can have listeners", I mean the .effect object. So, for instance, Dex.getAbility("Unburden") is an Ability, but Dex.getAbility("Unburden").effect is a PureEffect.
The Unburden Ability listens for item use, and when it happens, adds the Unburden PureEffect. The Unburden PureEffect doubles Speed.
Moves themselves don't have listeners, but they have PureEffects which have listeners.
Pokémon species (effectType === 'Template') themselves can have listeners – it's not used it regular simulation, but it's used by mods, including NEXT.
Other than that, I believe that's comprehensive.

how should inheriting and callbackTypes work? Given that it's always OK to make callbackTypes larger than expected (we just get less performance benefit) I erred on the side of caution and just copied everything from the parent.

Yes, that's safe.

is looking for startsWith('on') actually the most robust way to do this? Thats the heuristic I used when going over dev-tools/globals.ts to come up with the hardcoded CALLBACK_TYPES, but I'm not sure if that's right. Also, that nested for-loop is ugh.

It's probably how I'd do it. You'd also catch event handlers for singleEvents, but I don't see that causing any bugs.

how do I deal with the loading/test issue in an acceptable way? The naive solution for speedy tests would be could use the hardcoded set of CALLBACK_TYPES during testing and the loaded callbackTypes in prod, but then we're not actually testing reality so there's cracks for bugs to slip in.

I'm still confused about why loading is a problem in tests.

I'm (perhaps naively?) assuming they're all hardcoded, and aren't dynamically created on the fly, so everything should be under data or data/mods.

Yes, they're all hardcoded and immutable. Although note that they can also be under config/formats.js.

Abilities and Items are mostly straightfoward, but a bunch of things are in data/statuses (Statuses, Weather, some VolatileStatuses and SideConditions, and even Pokemon) - the problem here is that they don't all have a nice effectType which will be required for what I described in https://github.com/Zarel/Pokemon-Showdown/pull/5191#discussion_r258789987, so I think that the first step is probably going and adding that information to the data.

Anything without an effectType should probably be PureEffect. I'm pretty sure this includes all volatile statuses, side conditions, and field conditions (pseudoweathers). I don't remember how Pokemon listeners are implemented, but I think they're grafted into the template.

The other challenge is that other effects (like Terrain) are actually hidden away in nested effect fields of the EffectData type - so in order to gather them all up it'll require recursively traversing any Effect to its child effect.

Yes, this is the PureEffect system. It shouldn't require too much recursion – PureEffects themselves are guaranteed not to have .effect objects.

I don't imagine we chain these that deep, but it just makes this that much more fun :). These will also want to be given an effectType (even though the nested ones technically aren't required to have it because they're only Partial<EffectData>!)

You don't access the nested ones directly; you access them with e.g. Dex.getEffect('Unburden'), which will promote them to PureEffects.

I appreciate the detailed reply, thanks.

I don't understand. What makes tests fail because we're not loading data?

 Didn't notice that I needed to copy over my this.callbackTypes field in the section in the constructor which does a this.isOriginal check. Disregard that concern, sorry for the confusion.

I would use 'listener' rather than 'callback'.

Noted.

You have Formats in there twice. :p

Yeah, but it's a Set so...
;)

<...explanation of effects...>

Thanks - definitely good background, but I don't think this is entirely what I'm after. See below.

Although note that they can also be under config/formats.js.

Noted.

Anything without an effectType should probably be PureEffect. I'm pretty sure this includes all volatile statuses, side conditions, and field conditions (pseudoweathers). I don't remember how Pokemon listeners are implemented, but I think they're grafted into the template.

I'm less concerned about what is and what is not a pure effect, and instead in figuring out what is and what is not a callback on the places we're checking for listeners in getEventHandlers. Explicitly:

getPokemonEventHandlers: we check status, volatileStatuses, ability, item and species for listeners.
getBattleEventHandlers: we check pseudoWeathers, weather, terrain, format and events for listeners.
getSideEventHandlers: we check sideConditions for listeners.

While some of these things may or may not be a PureEffect, and while they all may have any possible type of listener, in practice, they have substantially different listeners depending on the type of thing we're talking about. For example, after 100 random battles (might have actually be 1000, but w/e), we observe the following sets of callbacks used by type:
ability: 69 = [onModifyAtk onModifySpA onModifySecondaries onTryHit onFoeBasePower onWeather onDeductPP onAfterDamage onDamage onResidual onBoost onModifyDef onSwitchOut onFlinch onAllyTryHitSide onSourceModifyDamage onModifyMove onModifySpe onSourceModifyAtk onSourceModifySpA onAfterUseItem onTakeItem onFoeTryEatItem onSourceFaint onModifyPriority onAfterSetStatus onSourceTryHeal onBasePower onCriticalHit onBeforeMove onSourceBasePower onUpdate onTryAddVolatile onHit onModifyCritRatio onSetStatus onAllyTryAddVolatile onAnyBasePower onAfterEachBoost onDragOut onFoeTrapPokemon onFoeMaybeTrapPokemon onAllyFaint onFoeTryMove onImmunity onSourceModifyAccuracy onAnyTryPrimaryHit onModifyDamage onAllyBoost onAllySetStatus onModifyAccuracy onAnyAccuracy onPrepareHit onAllyModifyAtk onAllyModifySpD onAllyModifyMove onAnySetWeather onSourceHit onAfterMoveSecondary onModifyBoost onBeforeSwitchIn onFaint onSourceModifySecondaries onEffectiveness onAnyModifyBoost onModifyWeight onEatItem onAnyFaint onAllyAfterUseItem]
item: 29 = [onResidual onTerrain onDamage onModifyMove onModifySpA onAfterMoveSecondarySelf onModifyDamage onBasePower onTakeItem onModifyAtk onHit onUpdate onTryHit onModifyDamagePhase2 onTryEatItem onModifySpe onAfterDamage onAfterSubDamage onModifyDef onModifySpD onDisableMove onChargeMove onSourceModifyDamage onModifyPriority onSourceTryPrimaryHit onModifyCritRatio onSwitchIn onTrapPokemon onEffectiveness]
status: 10 = [onSwitchIn onResidual onBeforeMove onModifySpe onModifyMove onHit onAfterMoveSelf onAfterSwitchInSelf onAfterMoveSecondary onAfterMoveSecondarySelf]
volatile: 36 = [onBeforeMove onDisableMove onTryHit onAllyTryHitSide onAfterMoveSecondarySelf onDamage onAfterMoveSecondary onTryPrimaryHit onResidual onLockMove onModifySpe onType onStallMove onOverrideAction onModifyDamagePhase1 onHit onNegateImmunity onModifyBoost onMoveAborted onImmunity onAfterMoveSelf onModifyAtk onModifySpA onModifyCritRatio onFaint onBeforeSwitchOut onTrapPokemon onBasePower onAfterDamage onTryImmunity onAnyTryMove onModifyWeight onSourceModifyDamage onSourceBasePower onUpdate onAfterSetStatus]
sideCondition: 8 = [onAnyModifyDamage onSwitchIn onAnyModifyDamagePhase1 onResidual onModifySpe onBeforeSwitchOut onSetStatus onTryAddVolatile]
weather: 6 = [onWeatherModifyDamage onImmunity onResidual onWeather onModifySpD onTryMove]
species: 1 = [onType]
terrain: 6 = [onTryHit onBasePower onSetStatus onTryAddVolatile onResidual onTerrain]

None are using all of the ~148 possible callbacks. What I hope to do is to compute a map of sets:
type Foo = 'Status' | 'VolatileStatus' | 'Ability' | 'Item' | 'Species' | 'PseudoWeather' | 'Weather' | 'Terain' | 'Format' | 'Event' // TODO: not sure about Event...

possibleCallbackTypes:  Map<Foo, Set<String>> = new Map();

Where Foo is I think EffectType, but effect seems to have a special meaning so I don't want to claim everything in Foo is or isn't an effect by accident. My point is, I want to build this possibleCallbackTypes up in Dex#loadData as we read in the data files (and config/format.js), potentially recursing through .effect fields to find them all. However, I need all the things I'm looking for listeners on to have an effectType or at least something which lets me know what kind of Foo they are so I can use that as the key for my map.
Once I have the map, I can get really specific in get*EventHandlers:
...
if (possibleCallbackTypes.get('PseudoWeather').has(callbackType)) {
  for (let i in this.pseudoWeather) {
    ...
  }
}
...

This change was originally hoping to come up with one set for possibleCallbackTypes (ie/ Battle#CALLBACK_TYPES), and thats still an option, but if I'm building it up dynamically instead of hardcoding it I might as well go all the way and get the breakdowns (lets only look at species for an onType listener instead of for the other 147 listeners it  never happens to have!). In https://github.com/Zarel/Pokemon-Showdown/pull/5191#discussion_r258789987 I said it was out of scope for this PR, but this PR is probably going to get trashed anyway after I rewrite it thanks to all the recent refactoring, and I think its pretty much just as easy (well, modulo having to annotate all the data with an effectType) to jump to the next stage than it is to try and first commit a less optimized step.

I think ideally the map should be the other way around? Where you have a map from EventName to the list of places to look for callbacks? Which would in theory be a list of callbackName/where-to-look tuples?
You might actually want to straight-up just compile the searcher from strings; JavaScript lets you do that with eval tricks. But that's probably unnecessarily fancy.

A first step might be to break up the individual find<X>EventHandlers functions.
So e.g.
``
let status = thing.getStatus();
// @ts-ignore
if (status[callbackName] !== undefined || (getKey && thing.statusData[getKey])) {
// @ts-ignore
handlers.push({status: status, callback: status[callbackName], statusData: thing.statusData, end: thing.clearStatus, thing: thing});
this.resolveLastPriority(handlers, callbackName);
}

would be its own function, etc etc

In hindsight, findPokemonEventHandlers is already probably short enough. Doing it purely by EventName : list of callbackNames should probably be sufficient.

Which would in theory be a list of callbackName/where-to-look tuples?

Doing it purely by a map from EventName to an array of [callbackName, pokemon/side/battle] tuples should probably be sufficient.

I think we're not on the same page quite yet - I want a mapping between 'place/slot/field listener could be within Pokemon/Side/Battle' (not the Pokemon/Side/Battle itself)  and 'callbackName' The 11 different places/slots/field that I'm interested in are bulleted in https://github.com/Zarel/Pokemon-Showdown/pull/5191#issuecomment-466216703.
What direction that mapping goes (place -> Set(<valid callbackNames>) like I originally proposed above, or callbackNames -> Set(<places>) like I was interpreting your suggestion before it was edited) is TBD, but if I can build the mapping I can benchmark both pretty trivially and see which one proves to be more beneficial (I'm guessing the latter). This is a further extension of the original design in  b531250 where I was just interested in creating a Set(<valid callbacks>).

A first step might be to break up the individual find<X>EventHandlers functions

I'm happy with the current structure of find{,Battle,Side,Pokemon}EventHandler functions already- you did an awesome job there with the refactoring to make them understandable.

You might actually want to straight-up just compile the searcher from strings; JavaScript lets you do that with eval tricks. But that's probably unnecessarily fancy.

Not entirely sure what you're talking about here, probably over my head wrt Javascript eval voodoo :). I think building the maps/sets I want is possible to accomplish with some pretty straightforward looping/recursion, provided I first go through all the data and give everything an effectType. WDYT?

I think I understand? Yes, that's pretty similar to the design I was thinking of, although I think my design would be slightly faster.
If you think of the current design as, in pseudocode:
findEventHandlers(pokemon, source) {
  findPokemonEventHandlers(pokemon, 'onEvent');
  for (each ally) {
    findPokemonEventHandlers(ally, 'onAllyEvent');
    findPokemonEventHandlers(ally, 'onAnyEvent');
  }
  ...
  findSideEventHandlers(pokemon.side, 'onEvent');
  findSideEventHandlers(pokemon.side, 'onAnyEvent');
  ...
}

If I'm understanding correctly, you want to change it to
findEventHandlers(pokemon, source) {
  if (cache.has('pokemon:onEvent')) findPokemonEventHandlers(pokemon, 'onEvent');
  if (cache.has('pokemon:onAllyEvent') || cache.has('pokemon:onAnyEvent')) for (each ally) {
    if (cache.has('pokemon:onAllyEvent')) findPokemonEventHandlers(ally, 'onAllyEvent');
    if (cache.has('pokemon:onAnyEvent')) findPokemonEventHandlers(ally, 'onAnyEvent');
  }
  ...
  if (cache.has('side:onEvent')) findSideEventHandlers(pokemon.side, 'onEvent');
  if (cache.has('side:onAnyEvent')) findSideEventHandlers(pokemon.side, 'onAnyEvent');
  ...
}

I'm thinking it might be better to do:
findEventHandlers(pokemon, source) {
  for (each toSearch in cache) {
    switch (toSearch) {
    case 'pokemon:onEvent':
      findPokemonEventHandlers(pokemon, 'onEvent');
      break;
    case 'pokemon:onAllyEvent':
      for (each ally) findPokemonEventHandlers(ally, 'onAllyEvent');
      break;
    case 'pokemon:onAnyEvent':
      for (each side) for (each active) findPokemonEventHandlers(pokemon, 'onAnyEvent');
      break;
    ...
    case 'side:onEvent':
      findSideEventHandlers(pokemon.side, 'onEvent');
      break;
    case 'side:onAnyEvent':
      findSideEventHandlers(pokemon.side, 'onAnyEvent');
      findSideEventHandlers(pokemon.side.foe, 'onAnyEvent');
      break;
    ...
    }
    }
  }
}

Actually, now that I've written out the pseudocode, I'm less confident there would be perf differences with the two approaches.
I do see how manually compiling each one would be faster, though.
Imagine:
Say we know for sure poke:onDamage and side:onAnyDamage exist, but the others don't. We could use eval tricks to define this function:
findDamageEventHandlers(pokemon, source) {
  findPokemonEventHandlers(pokemon, 'onDamage');
  findSideEventHandlers(pokemon.side, 'onAnyDamage');
  findSideEventHandlers(pokemon.side.foe, 'onAnyDamage');
}

and save it in a cache for the Damage event.

I think I see where we're not on the same page, and I think I'm to blame - sorry. There's really two things going on in b531250:

it skips checking various fields ('places' above:  status, volatileStatuses, ability, item species, pseudoWeathers, weather, terrain, format, "events",  sideConditions) once inside a find<X>EventHandler
it avoid recursive calls (or now, after the refactoring, calls to find<Y>EventHandler methods)

With one big CALLBACK_TYPES set like I hardcoded in b531250, distinguishing between these is unimportant. Even this large set netted a 20-25% improvement, but I figured we could go further.
To further optimize, I wanted a mapping between fields <-> valid callback names, forgetting about needing to also determine whether we could skip recursive calls (likely the more important thing to skip). To accomplish this I believe we'd actually need a mapping between (Pokemon|Side|Battle|field) <-> valid callback names.
field <-> callback name is pretty straightforward if all effects for those fields have an effectType. Not sure how to do better for Pokemon, Side, or Battle than just the union of all possible callbacks (ie/ CALLBACK_TYPES, but generated dynamically) - maybe this is what you were trying to tell me above?
https://github.com/Zarel/Pokemon-Showdown/pull/5191#issuecomment-466262674 - the first two pseudocode blocks here make sense to me - the middle is essentially what I'm proposing, though as I just mentioned, i'm not sure how to determine 'poke:', 'battle:' or 'side:' callbacks, so I'd probably just have one cache for all of those - I'm interested in generating 'species:onType' or pseudoWeather:onFoo as well, but your pseudocode doesnt go that deep. The third example lost me - why would I be looping through the entire cache? Is it already constrained in some way?
Manually compiling each combination seems like it could be faster (though costs in code size), but I'm not aware of any eval tricks (eval is already a performance nightmare, and I've always stayed clear of it). I'm also uneasy about using 'magic' here - while it'd no doubt be cool, and probably a bit faster than what I'm proposing, it also seems like it could be overkill? Or at least make the code less tractable for other developers to understand and edit? I'm fully willing to admit I could be completely wrong here - as I said, I don't know what kind of cool things you can do with eval, but I don't think its something I'd personally be comfortable taking on.
My proposal:

First we come up with code to find all the listeners and what they could be on. No matter what solution we end with (if (cache.has)..., switch... or eval), it seems like we'd need to be able to do this.
Implement the cache.has approach similar to what b531250 does: it doesn't really change the code structure that much, is pretty easy to understand, and nets us a proven win.
We can then experiment with going further - maybe the code can be both cleaner, more understandable and faster with eval. Great, i'd be all for that.

inb4 i go to bed and wake up tomorrow finding youve already gone galaxy brain on me and jumped ahead to step 3 like last night... ;)

i'm not sure how to determine 'poke:', 'battle:' or 'side:' callbacks, so I'd probably just have one cache for all of those

You're right, I don't know of a good way of doing that.

I'm interested in generating 'species:onType' or pseudoWeather:onFoo as well

I understand. Although for the same reason you can't tell poke: battle: side: apart, you can't tell pseudoWeather apart from volatile and all those.

The third example lost me - why would I be looping through the entire cache? Is it already constrained in some way?

It would replace a bunch of tests/branches with a single loop. It's the difference between
let cache = new Set(['q', 'r', 's']);
// ...
for (const char of alphabet) {
  if (cache.has(char)) console.log(char);
}

and
let cache = ['q', 'r', 's'];
// ...
for (const char of cache) {
  console.log(char);
}

Manually compiling each combination seems like it could be faster (though costs in code size), but I'm not aware of any eval tricks (eval is already a performance nightmare, and I've always stayed clear of it).

The eval trick equivalent of the above is:
let cachedLetters = ['q', 'r', 's'];
let code = '';
for (const char of cachedLetters) {
  code += `console.log(${JSON.stringify(char)});`;
}
let cache = new Function(code);
// ...
cache();

Which will be the absolute best-performing of all of these solutions. This has various disadvantages and isn't allowed by secure runtimes, and is a lot of effort, but it's the galaxy-brain approach.
I'm too lazy to write the code for it, though. You're on your own for that one. :p

I'm interested in generating 'species:onType' or pseudoWeather:onFoo as well

I understand. Although for the same reason you can't tell poke: battle: side: apart, you can't tell pseudoWeather apart from volatile and all those.

We can't currently, but that's why I'm proposing I go and actually manually add effectType to all the effects in the data files. Then I can distinguish the sideConditions from volatiles in data/statuses.js, or that the 'electricterrain' move's effect is a pseudoWeather.

The third example lost me - why would I be looping through the entire cache? Is it already constrained in some way?

It would replace a bunch of tests/branches with a single loop. It's the difference between
let cache = new Set(['q', 'r', 's']);
// ...
for (const char of alphabet) {
  if (cache.has(char)) console.log(char);
}

and
let cache = ['q', 'r', 's'];
// ...
for (const char of cache) {
  console.log(char);
}

Yeah, except we're not looping through an 'alphabet' of callback types (or maybe we are? just at a higher level?). From the context of findEventHandlers theres one callbackType param, not a whole list of them. Are you talking about somewhere higher in the event system? I've been narrowly focused - maybe you're already a couple steps ahead of me here?

The eval trick equivalent of the above is:
let cachedLetters = ['q', 'r', 's'];
let code = '';
for (const char of cachedLetters) {
  code += `console.log(${JSON.stringify(char)});`;
}
let cache = new Function(code);
// ...
cache();

Which will be the absolute best-performing of all of these solutions. This has various disadvantages and isn't allowed by secure runtimes, and is a lot of effort, but it's the galaxy-brain approach.

Gotcha. Cool. Not sure I'd need to take it that far - I'll probably do another profile analysis after I land this optimization and #5194 and see whether its still worth the investment here or if there are other areas worth looking into.

We can't currently, but that's why I'm proposing I go and actually manually add effectType to all the effects in the data files. Then I can distinguish the sideConditions from volatiles in data/statuses.js, or that the 'electricterrain' move's effect is a pseudoWeather.

Oh. I'm not opposed to that.

Yeah, except we're not looping through an 'alphabet' of callback types (or maybe we are? just at a higher level?). From the context of findEventHandlers theres one callbackType param, not a whole list of them. Are you talking about somewhere higher in the event system? I've been narrowly focused - maybe you're already a couple steps ahead of me here?

I'm not sure what you mean. I'm definitely equally narrowly focused and not talking about changing anything outside of findEventHandlers. callbackType no longer exists as a variable name; I think it's been consistently renamed callbackName – or do you mean eventName?
Although, if we wanted to step back, one other optimization would be to actually register and deregister event handlers when volatile statuses, items, etc are gained and lost, instead of keeping a global cached list.

I'm not sure what you mean. I'm definitely equally narrowly focused and not talking about changing anything outside of findEventHandlers. callbackType no longer exists as a variable name; I think it's been consistently renamed callbackName – or do you mean eventName?

Right. I meant eventName or callbackName (which is just on${eventName}, right?). Sorry for the confusion.

Although, if we wanted to step back, one other optimization would be to actually register and deregister event handlers when volatile statuses, items, etc are gained and lost, instead of keeping a global cached list.

Yeah. That's originally what I proposed in pkmn.cc/optimize, and how I got to here :).
If you look at the pseudocode in that doc you'll see it loops through something called 'CALLBACK_TYPES' in order to register and deregister event handlers (its poorly named, it should be VALID_CALLBACK_NAMES or something). In trying to determine what valid callbackNames there could be I determined that the method was getting called with over 400 different callbackNames. Looping through ~400 names to check if we need to register(/deregister) listeners each time a status/volatile/side condition/etc is added(/removed) didn't seem to be super efficient, and thats when I realized that only ~150/400 aren't bogus, and for some types that's even smaller (species being my favorite to pick on, because it only seems to use onType for Arceus/Sivally).
I still think the registration approach is going to be the architecture with the best performance. What I was trying to do on this branch was an intermediate state (which might have been good enough, and only involved changing 1 method at the time in getRelevantEffectsInner instead of the ~2*11 different methods where registration/deregistration would need to occur). The registration approach faces the same problem though: unless I'm missing something, for each type which we need to check for registerable listeners on we need to know the set of possible callbacks that type can have.

I don't think registration is that hard. I'm going to take a stab at it.

(callbackName is one of on${eventName}, onSource${eventName}, onAlly${eventName}, onFoe${eventName}, or onAny${eventName}.)

I don't think its hard, but can you do it without touching 22 methods (reg and dereg methods for each of the fields)? And knowing the callback types for possible for each field?
Anwyay, I'd love if you wanted to take a stab at it :)! In my experience with performance work at my day job it's usually less value for me to be the one writing code anyway - it's almost always better for me to simply shine my flashlight at where problems are and help the original author come to the best solution using their expertise :).

What do you mean by "each of the fields"? It'd touch a lot of methods, like addVolatile, removeVolatile, addSideCondition, removeSideCondition, etc. Yes, probably 22 overall.

And I don't think you'd need to know callback types or anything like that.

What do you mean by "each of the fields"?

Yep, by 'fields' I mean:
status, volatileStatuses, ability, item, species, pseudoWeathers, weather, terrain, format, "events",  sideConditions

I agree, it's very tractable, but changing 1 method is less intrusive and as an outsider its usually easier to get consent to optimize a method than it is to change 22 different places in a new codebase :)

And I don't think you'd need to know callback types or anything like that.

Alright, when you register an ability, how do you know which methods to look for? You just loop over all of the effect object's keys and look for function objects (or maybe actually for keys starting with 'on'?). That approach would work pretty well, but its still a little less optimized than it could be in some cases (Most effects have 19 fields they all inherit that aren't listeners, for example. Or Species/template has a bunch more fields than just the 'onType' we are looking for). Though actually, in many case the number of keys on the object may be less than the number of possible callbacks we're looking for, and in the cases where it isn't we could measure if it actually matters and add special optimizations for that (like Species + onType if it actually mattered, which it probably doesnt)
OK, I think we're finally on the same page! You're going to take this on? Or did you want me to take a stab?
(Thanks for putting up with all these questions and confusion!)

You just loop over all of the effect object's keys and look for function objects (or maybe actually for keys starting with 'on'?)

Yeah, that's the plan, look for keys starting with on. I imagine it wouldn't be that bad for perf.

OK, I think we're finally on the same page! You're going to take this on? Or did you want me to take a stab?

I'll write an implementation for volatiles, to see if there are any major barriers I'd need to deal with. Once that's in, I'll let you use that as a model for the other "fields".

Sure, sounds good!

I'm glad I started with volatiles; there are way more special cases to deal with.

https://github.com/Zarel/Pokemon-Showdown/commit/f0d0fe009158e829d2f1ed804c176d0f2458fe17
It took me like five hours, but I have a stub. It's missing Baton Pass support.
Did you know we have zero tests for Baton Passing volatiles? We have zero tests for switch-outs and volatiles in general. I had to write https://github.com/Zarel/Pokemon-Showdown/commit/9b694fa350b1ba32024b41e5fee8b575ced1069d because I was weirded out that tests passed before I implemented clearing volatiles after switch-out.

Also, my stub implementation does not seem to have a noticeable perf difference, which is worrying.

Also, my stub implementation does not seem to have a noticeable perf difference, which is worrying.

How are you measuring? Also, its 1/11th of the possible ListenerTypes (though probably the most important)

How are you measuring?

I'm just timing mocha. My getRelevantEffects -> findEventListeners refactor brought it down from 11s to 10s on my computers, but this change has not changed it from 10s.

Also, its 1/11th of the possible ListenerTypes (though probably the most important)

It's likely the most important. I guess the main win might be to skip findEventListeners entirely if the table's empty.

I can try patching and running it through my framework to see if there are more granular changes (which maybe I should look into checking in under some subdirectory somewhere to make things easier)?  Expecting to drop another second would be pretty substantial...

It's likely the most important. I guess the main win might be to skip findEventListeners entirely if the table's empty.

Yeah - being able to skip completely non existent eventNames is worth 20%-25% with the getRelevantEventsInner architecture. I would expect that to be the absolute floor on the size of improvement we would expect from the refactored and improved architecture.

Oops, was doing some branch cleanup and closed this. Not that I plan on merging this given I assume we're going to go forward with the new architecture, but I might as well leave this open until we get there.

f0d0fe0
It took me like five hours, but I have a stub. It's missing Baton Pass support.
Did you know we have zero tests for Baton Passing volatiles? We have zero tests for switch-outs and volatiles in general. I had to write 9b694fa because I was weirded out that tests passed before I implemented clearing volatiles after switch-out.

Have you had a chance to look into working on this again? I'm happy to help: maybe I can write some tests for passing volatiles with Baton Pass so you can get back to the actual implementation, or fix any merge issues to get the branch back to HEAD, or benchmarking so we better understand its effect, etc?

I already wrote the relevant tests. I'm mostly done working on it at this point; feel free to pick it up where I left off.

While I plan to come back to optimizing the event system using registration instead of searching at some point in the (potentially not so near) future, I will be basing it more off of f0d0fe0 than this PR, so I'm closing it.

Should we make an issue for f0d0fe009158e82?

I had it on my own list, but no reason not to make an issue for it. :)


JDBC Writer

Pull request for JDBC Writer. Will combine to one commit after the review.
Design doc: https://docs.google.com/a/linkedin.com/document/d/1pSWZtgNssxaV1c19xdd1rKft9JaXM2qmqDwwbIb0scM/edit?usp=sharing

Coverage increased (+0.7%) to 42.527% when pulling 19e7dbda11ec7915632d5d3edcbf76dc8b71c5ed on jinhyukchang:master into f5573e120330d0472c47f2690d222581a74724e1 on linkedin:master.

Any reason the design doc can't be public?

Coverage increased (+1.5%) to 43.319% when pulling ac1de186a15ea2604d64360b60efb184f38d83b2 on jinhyukchang:master into f5573e120330d0472c47f2690d222581a74724e1 on linkedin:master.

@kadaan Internally, for reviews, we use a company google docs account. Once we finalize the internal review, we'll share the design.

Update the code per PR.

Merged from origin to sync up to date.

Coverage increased (+0.8%) to 45.133% when pulling 67598dae00afee0e6579ecefbb8e71af2e04acd0 on jinhyukchang:master into 911ab2eb57a36a6cacc373f4f3f409fba5adb257 on linkedin:master.

@sahilTakiar Just updated the code addressing PR. Would you take a look when you have a chance?

Coverage increased (+0.6%) to 44.845% when pulling 09ffbfcf0b65fb34a9c019b76947911a2abeacef on jinhyukchang:master into 911ab2eb57a36a6cacc373f4f3f409fba5adb257 on linkedin:master.

hey @jinhyukchang I discussed with @chavdar and everything in this PR looks good! I can merge this PR, but I have one last request.
Can you add some documentation to the Gobblin Docs about this feature, you can probably just copy what you have in your Google Doc. If you have time its always nice to add documentation on configuration keys and documentation on the different plugin points. This is generally useful for any new Gobblin user who wants to write to a RDMS (I already know of one user waiting on this PR because they want to use this).
All Gobblin documentation is checked into GitHub under gobblin-docs. I suggest putting the documentation in a new file called gobblin-docs/user-guide/Gobblin-JDBC-Writer.md. The format of the files is all in Markdown. It's should be a relatively simple process, but if you have more questions this page has more details: http://gobblin.readthedocs.io/en/latest/developer-guide/Contributing/

@jinhyukchang we will feature the JDBC writer as one of the highlights for our next release (0.7.0) so it will be great to have some supporting documentation.

@sahilTakiar , @chavdar ,
Great, thanks for the review and will add documentation shortly.

@sahilTakiar , @chavdar
Just added documentation. Let me know how it looks.
Seems now there's a conflict with master branch and I will resolve it once documentation is reviewed.

Documentation LGTM!
When you rebase this PR, can you also squash all the commits together into one commit?

Sure, will do that.

Coverage increased (+0.5%) to 45.648% when pulling 387da5a90ff94f80dac0b09514ccc5d2047fd09b on jinhyukchang:master into 9971e736a6eb3328e2375875ad362c87ff574678 on linkedin:master.

Coverage increased (+0.1%) to 45.369% when pulling 43d40e087054bde28eaff17743d9bfc0475efcda on jinhyukchang:master into 73e8720993748a371f0517a09675285876e97562 on linkedin:master.


ISSUE: User Locks are not functioning at all

Describe the bug
New installation of Schlage z-wave lock and HA.  The lock functions in terms of locking via HA basic z-wave functionality.  When Keymaster is implemented, the locks are not able to be configured and it appears no user codes can be configured.
Environment (please complete the following information):
System Health

version
core-2022.4.1

installation_type
Home Assistant OS

dev
false

hassio
true

docker
true

user
root

virtualenv
false

python_version
3.9.9

os_name
Linux

os_version
5.10.103-v8

arch
aarch64

timezone
America/New_York

Home Assistant Community Store

GitHub API
ok

GitHub Content
ok

GitHub Web
ok

GitHub API Calls Remaining
4886

Installed Version
1.24.5

Stage
running

Available Repositories
1099

Downloaded Repositories
7

Home Assistant Cloud

logged_in
false

can_reach_cert_server
ok

can_reach_cloud_auth
ok

can_reach_cloud
ok

Home Assistant Supervisor

host_os
Home Assistant OS 7.6

update_channel
stable

supervisor_version
supervisor-2022.04.0

docker_version
20.10.9

disk_total
29.1 GB

disk_used
5.1 GB

healthy
true

supported
true

board
rpi4-64

supervisor_api
ok

version_api
ok

installed_addons
Z-Wave JS (0.1.57), chrony (2.3.0), SSH & Web Terminal (10.1.2), Let's Encrypt (4.12.2)

keymaster

zwave_integration
zwave_js

network_status
on

Dashboards

dashboards
2

resources
5

views
1

mode
storage

Z-Wave integration name: zwave_js
Lock make and model: Schlage BE469ZP

Logs
2022-04-30 16:23:11 WARNING (MainThread) [homeassistant.helpers.service] Unable to find referenced entities timer.keymaster_frontdoor_deadbolt_autolock
2022-04-30 16:23:11 WARNING (MainThread) [homeassistant.helpers.service] Unable to find referenced entities script.keymaster_frontdoor_deadbolt_start_timer
2022-04-30 16:29:01 ERROR (MainThread) [custom_components.keymaster.services] Error calling zwave_js.set_lock_usercode service call: userCode for code slot 6 not found
2022-04-30 16:29:01 ERROR (MainThread) [homeassistant.components.automation.keymaster_synchronize_codeslot_frontdoor_deadbolt_6] keymaster_synchronize_codeslot_frontdoor_deadbolt_6: Choose at step 1: choice 1: Error executing script. Unexpected error for call_service at pos 1: userCode for code slot 6 not found
Traceback (most recent call last):
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 379, in _async_step
await getattr(self, handler)()
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 582, in _async_call_service_step
await service_task
File "/usr/src/homeassistant/homeassistant/core.py", line 1634, in async_call
task.result()
File "/usr/src/homeassistant/homeassistant/core.py", line 1671, in _execute_service
await cast(Callable[[ServiceCall], Awaitable[None]], handler.job.target)(
File "/config/custom_components/keymaster/init.py", line 224, in _add_code
await add_code(hass, entity_id, code_slot, usercode)
File "/config/custom_components/keymaster/services.py", line 155, in add_code
await call_service(
File "/config/custom_components/keymaster/services.py", line 94, in call_service
raise err
File "/config/custom_components/keymaster/services.py", line 89, in call_service
await hass.services.async_call(
File "/usr/src/homeassistant/homeassistant/core.py", line 1634, in async_call
task.result()
File "/usr/src/homeassistant/homeassistant/core.py", line 1671, in _execute_service
await cast(Callable[[ServiceCall], Awaitable[None]], handler.job.target)(
File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 692, in handle_service
await service.entity_service_call(
File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 677, in entity_service_call
future.result() # pop exception if have
File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 955, in async_request_call
await coro
File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 714, in _handle_entity_call
await result
File "/usr/src/homeassistant/homeassistant/components/zwave_js/lock.py", line 129, in async_set_lock_usercode
await set_usercode(self.info.node, code_slot, usercode)
File "/usr/local/lib/python3.9/site-packages/zwave_js_server/util/lock.py", line 112, in set_usercode
value = get_code_slot_value(node, code_slot, LOCK_USERCODE_PROPERTY)
File "/usr/local/lib/python3.9/site-packages/zwave_js_server/util/lock.py", line 32, in get_code_slot_value
raise NotFoundError(f"{property_name} for code slot {code_slot} not found")
zwave_js_server.exceptions.NotFoundError: userCode for code slot 6 not found
2022-04-30 16:29:01 ERROR (MainThread) [homeassistant.components.automation.keymaster_synchronize_codeslot_frontdoor_deadbolt_6] keymaster_synchronize_codeslot_frontdoor_deadbolt_6: Error executing script. Unexpected error for choose at pos 1: userCode for code slot 6 not found
Traceback (most recent call last):
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 379, in _async_step
await getattr(self, handler)()
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 761, in _async_choose_step
await self._async_run_script(script)
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 838, in _async_run_script
await self._async_run_long_action(
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 546, in _async_run_long_action
long_task.result()
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 1275, in async_run
await asyncio.shield(run.async_run())
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 359, in async_run
await self._async_step(log_exceptions=False)
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 379, in _async_step
await getattr(self, handler)()
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 582, in _async_call_service_step
await service_task
File "/usr/src/homeassistant/homeassistant/core.py", line 1634, in async_call
task.result()
File "/usr/src/homeassistant/homeassistant/core.py", line 1671, in _execute_service
await cast(Callable[[ServiceCall], Awaitable[None]], handler.job.target)(
File "/config/custom_components/keymaster/init.py", line 224, in _add_code
await add_code(hass, entity_id, code_slot, usercode)
File "/config/custom_components/keymaster/services.py", line 155, in add_code
await call_service(
File "/config/custom_components/keymaster/services.py", line 94, in call_service
raise err
File "/config/custom_components/keymaster/services.py", line 89, in call_service
await hass.services.async_call(
File "/usr/src/homeassistant/homeassistant/core.py", line 1634, in async_call
task.result()
File "/usr/src/homeassistant/homeassistant/core.py", line 1671, in _execute_service
await cast(Callable[[ServiceCall], Awaitable[None]], handler.job.target)(
File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 692, in handle_service
await service.entity_service_call(
File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 677, in entity_service_call
future.result() # pop exception if have
File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 955, in async_request_call
await coro
File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 714, in _handle_entity_call
await result
File "/usr/src/homeassistant/homeassistant/components/zwave_js/lock.py", line 129, in async_set_lock_usercode
await set_usercode(self.info.node, code_slot, usercode)
File "/usr/local/lib/python3.9/site-packages/zwave_js_server/util/lock.py", line 112, in set_usercode
value = get_code_slot_value(node, code_slot, LOCK_USERCODE_PROPERTY)
File "/usr/local/lib/python3.9/site-packages/zwave_js_server/util/lock.py", line 32, in get_code_slot_value
raise NotFoundError(f"{property_name} for code slot {code_slot} not found")
zwave_js_server.exceptions.NotFoundError: userCode for code slot 6 not found
2022-04-30 16:29:01 ERROR (MainThread) [homeassistant.components.automation.keymaster_synchronize_codeslot_frontdoor_deadbolt_6] While executing automation automation.keymaster_synchronize_codeslot_frontdoor_deadbolt_6
Traceback (most recent call last):
File "/usr/src/homeassistant/homeassistant/components/automation/init.py", line 515, in async_trigger
await self.action_script.async_run(
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 1275, in async_run
await asyncio.shield(run.async_run())
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 359, in async_run
await self._async_step(log_exceptions=False)
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 379, in _async_step
await getattr(self, handler)()
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 761, in _async_choose_step
await self._async_run_script(script)
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 838, in _async_run_script
await self._async_run_long_action(
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 546, in _async_run_long_action
long_task.result()
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 1275, in async_run
await asyncio.shield(run.async_run())
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 359, in async_run
await self._async_step(log_exceptions=False)
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 379, in _async_step
await getattr(self, handler)()
File "/usr/src/homeassistant/homeassistant/helpers/script.py", line 582, in _async_call_service_step
await service_task
File "/usr/src/homeassistant/homeassistant/core.py", line 1634, in async_call
task.result()
File "/usr/src/homeassistant/homeassistant/core.py", line 1671, in _execute_service
await cast(Callable[[ServiceCall], Awaitable[None]], handler.job.target)(
File "/config/custom_components/keymaster/init.py", line 224, in _add_code
await add_code(hass, entity_id, code_slot, usercode)
File "/config/custom_components/keymaster/services.py", line 155, in add_code
await call_service(
File "/config/custom_components/keymaster/services.py", line 94, in call_service
raise err
File "/config/custom_components/keymaster/services.py", line 89, in call_service
await hass.services.async_call(
File "/usr/src/homeassistant/homeassistant/core.py", line 1634, in async_call
task.result()
File "/usr/src/homeassistant/homeassistant/core.py", line 1671, in _execute_service
await cast(Callable[[ServiceCall], Awaitable[None]], handler.job.target)(
File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 692, in handle_service
await service.entity_service_call(
File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 677, in entity_service_call
future.result() # pop exception if have
File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 955, in async_request_call
await coro
File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 714, in _handle_entity_call
await result
File "/usr/src/homeassistant/homeassistant/components/zwave_js/lock.py", line 129, in async_set_lock_usercode
await set_usercode(self.info.node, code_slot, usercode)
File "/usr/local/lib/python3.9/site-packages/zwave_js_server/util/lock.py", line 112, in set_usercode
value = get_code_slot_value(node, code_slot, LOCK_USERCODE_PROPERTY)
File "/usr/local/lib/python3.9/site-packages/zwave_js_server/util/lock.py", line 32, in get_code_slot_value
raise NotFoundError(f"{property_name} for code slot {code_slot} not found")
zwave_js_server.exceptions.NotFoundError: userCode for code slot 6 not found
Screenshots

 userCode for code slot 6 not found
Re-interview your lock, your code slots are missing.

I had the same problem with my Schlage BE469ZP's back on an older version of zwave_js. For me this problem is ended up being caused by an incomplete inclusion when using S0. For whatever reason, my locks just refused to properly transmit the usercodes during the inclusion process. After zwave_js added support for S2, I re-included my locks using S2 and this problem went away entirely.
It might be worth excluding/reincluding to verify that S2 security is used instead of the legacy S0

It appears the zwave js integration doesn't know how to work with the BE469ZP.  I had it connected with zwave plus, after about 4-5 re-interviews...it lost the S2 code.  Now it doesn't see the battery level and other sensors.  The lock still works via the unlock/lock via HA...but it still doesn't work with Keymaster.  I think it is time to dump Schlage...I have had no problems with them in the past, which is sad.

How many mains powered zwave nodes do you have?
This looks like signal issues.

I have an AEON z-stick ZW090 around 25-30 feet about from the front door lock.  I have two z-wave switches (Enlighten 46203) for my kitchen lighting system...those are right in the middle about 10-15 feet from the stick and front door.  I have an outlet (Minoston MP21Z) right next to the front door...literally 1 foot from the smart lock on the same wall.  The outlet works perfectly and never shows as being disconnected.

You can try to do a heal on the node.

So I am having a very similar problem to this. My node is staying secure though, but the User Codes are not being pulled into Home Assistant on the Lock. However, I can run the refresh codes script that KeyMaster provides and it does go and check all of the codes.

Check your home-assistant.log for errors. If you are getting the userCode for code slot X not found message, your lock is missing the User Codes command class in zwavejs.

So I don't know if I resolved it or not...but it does seem to be working now.
I'm seeing these messages in the logs:
2022-05-09 15:17:44 DEBUG (MainThread) [custom_components.keymaster] DEBUG: Code slot 28 not enabled
2022-05-09 15:17:45 ERROR (MainThread) [custom_components.keymaster] Unexpected error fetching keymaster data: 'NoneType' object is not subscriptable
Traceback (most recent call last):
File "/usr/src/homeassistant/homeassistant/helpers/update_coordinator.py", line 191, in _async_refresh
self.data = await self._async_update_data()
File "/usr/src/homeassistant/homeassistant/helpers/update_coordinator.py", line 150, in _async_update_data
return await self.update_method()
File "/config/custom_components/keymaster/init.py", line 602, in async_update_usercodes
return await self._async_update()
File "/config/custom_components/keymaster/init.py", line 639, in _async_update
usercode_resp = await get_usercode_from_node(node, code_slot)
File "/usr/local/lib/python3.9/site-packages/zwave_js_server/util/lock.py", line 105, in get_usercode_from_node
ATTR_IN_USE: resp["userIdStatus"] == CodeSlotStatus.ENABLED,
TypeError: 'NoneType' object is not subscriptable
2022-05-09 15:17:45 DEBUG (MainThread) [custom_components.keymaster] Finished fetching keymaster data in 8.725 seconds (success: False)
2022-05-09 15:17:50 DEBUG (MainThread) [custom_components.keymaster] DEBUG: Code slot 1 value: #### (Prog code 1)
2022-05-09 15:17:50 DEBUG (MainThread) [custom_components.keymaster] DEBUG: Code slot 2 value: #### (Prog code 2)
2022-05-09 15:17:50 DEBUG (MainThread) [custom_components.keymaster] DEBUG: Code slot 3 not enabled
2022-05-09 15:17:50 DEBUG (MainThread) [custom_components.keymaster] DEBUG: Code slot 4 not enabled
2022-05-09 15:17:50 DEBUG (MainThread) [custom_components.keymaster] DEBUG: Code slot 5 not enabled
2022-05-09 15:17:50 DEBUG (MainThread) [custom_components.keymaster] DEBUG: Code slot 6 not enabled
2022-05-09 15:17:50 DEBUG (MainThread) [custom_components.keymaster] DEBUG: Code slot 7 not enabled
2022-05-09 15:17:50 DEBUG (MainThread) [custom_components.keymaster] DEBUG: Code slot 8 not enabled
2022-05-09 15:17:50 DEBUG (MainThread) [custom_components.keymaster] DEBUG: Code slot 9 not enabled
I was able to add new codes and they showed up in the right slots.
I decided to try a lower Security level so now I'm connecting at a lower level.  Apparently, zwavejs doesn't support that security level as mature as they should for that device.

Thank you for the idea to try something different.


Received error type for string array is different than expected

I'm using version 3.0.0-alpha.4 and defining my schema as:
const postSchema = z.object({
    message: z.string().min(10),
    tags: z.string().array().max(3),
});

The type suggested by VSCode for errors.tags is (FieldError | undefined)[] | undefined, however, if zod gives me FieldError | undefined.

Not sure what errors or FieldError are - it's not a type that exists in the Zod codebase. Are you using some other form library in conjunction with Zod?

@colinhacks you're right. It is in react-hook-form's resolvers. :facepalm:
I really am sorry for posting it here.
Btw, thank you for this awesome library.


Remove reference to deprecated easy_install; use pip instead

easy_install is deprecated and its use is discouraged by PyPA:
https://setuptools.readthedocs.io/en/latest/easy_install.html

Warning: Easy Install is deprecated. Do not use it. Instead use pip.

Follow upstream advice and only recommended supported tools. Replace
with pip.

Updated to remove the tarball section
Also removed mentioning easy_install from the "MarkupSafe Dependency" section