Improve UpdateChecker #18101

apc999 · 2023-09-04T07:33:36Z

What changes are proposed in this pull request?

Improve UpdateChecker

Why are the changes needed?

remove irrelevant master features
turn on fuse update check

apc999 · 2023-09-04T07:34:48Z

dora/integration/fuse/src/main/java/alluxio/fuse/options/FuseOptions.java

@@ -31,17 +31,17 @@
 public class FuseOptions {
  private static final Logger LOG = LoggerFactory.getLogger(FuseOptions.class);
  public static final PropertyKey FUSE_UPDATE_CHECK_ENABLED =
-      PropertyKey.Builder.booleanBuilder("fuse.update.check.enabled")
+      PropertyKey.Builder.booleanBuilder("alluxio.fuse.update.check.enabled")


@dbw9580 is this purposely named as fuse.update.check.enabled rather than alluxio.fuse.update.check.enabled ?

The rationale for not defining it in PropertyKey.java and not starting with alluxio. is that this is purely a command line option, and it should not be made accessible in alluxio-site.properties, etc. I named it without the alluxio prefix and put it here so that the usage should be limited to fuse CLI facilities, hopefully.

oh, i see. so it is not a real property key but only a CLI option.
In this case, we shall not free-ride PropertyKey for this purpose.
I will leave your code unchanged in this PR, but let's find a different way to incorporate a CLI option

The CLI can be removed and not allow user to disable it from this approach.
do we consider using

dev/scripts/src/alluxio.org/build/profiles.yml: mvnArgs: "-Dupdate.check.enabled=false" docs/_data/generated/en/master-configuration.yml:alluxio.master.update.check.enabled: docs/_data/generated/master-configuration.csv:alluxio.master.update.check.enabled,"true" dora/core/common/src/main/java-templates/alluxio/ProjectConstants.java: public static final String UPDATE_CHECK_ENABLED = "${update.check.enabled}"; dora/core/common/src/main/java/alluxio/conf/PropertyKey.java: "alluxio.master.update.check.enabled"; dora/integration/fuse/src/main/java/alluxio/fuse/options/FuseOptions.java: PropertyKey.Builder.booleanBuilder("fuse.update.check.enabled") pom.xml: <update.check.enabled>true</update.check.enabled>

the update check maven arguments way for this fuse update check disable during internal development and testing?

i am also worried about hardcoding this. there needs to be a way to differentiate whether or not the update checks should run by default. the current mechanism is to set -Dupdate.check.enabled=false when building the code for specific purposes; see https://github.com/Alluxio/alluxio/blob/672562bc96b3bb575ad66e3d8e765ca2794b089e/dev/scripts/src/alluxio.org/build/profiles.yml#L127C1-L127C1

i recommend reusing the same mechanism and set .setDefaultValue(Boolean.parseBoolean(ProjectConstants.UPDATE_CHECK_ENABLED)) for this property

followed what @Xenorith suggested

apc999 · 2023-09-04T07:35:12Z

dora/integration/fuse/src/main/java/alluxio/fuse/options/FuseOptions.java

          .buildUnregistered();

  /**
   * The UFS root that Fuse mounts.
   * In standalone Fuse SDK, this is different from {@link PropertyKey#DORA_CLIENT_UFS_ROOT}.
   * */
  public static final PropertyKey FUSE_UFS_ROOT =
-      PropertyKey.Builder.stringBuilder("fuse.ufs.root")
+      PropertyKey.Builder.stringBuilder("alluxio.fuse.ufs.root")


@dbw9580 will this rename create issue?

Renaming this should not cause breakage in functionality, but I purposely did not follow the naming convention to indicate that this a property should be limited to the fuse component, and should not be used elsewhere.

LuQQiu · 2023-09-05T18:47:05Z

@Xenorith Add rico as the gate keeper before merging this PR

LuQQiu · 2023-09-05T18:51:28Z

Discussed with @Xenorith that we will add a ProcessType: FUSE to the checker
and generate the example result to him before merging this PR

apc999 · 2023-09-06T07:33:00Z

dora/integration/fuse/src/main/java/alluxio/fuse/meta/FuseUpdateChecker.java

+      mFuseInfo.add(LOCAL_KERNEL_DATA_CACHE);
+    }
+    mFuseInfo.add(String.format("UnderlyingFileSystem:%s", getUnderlyingFileSystem(fuseOptions)));
+    mFuseInfo.add(TYPE_FUSE);


@Xenorith 👆

Xenorith

previously the update checker on master has a "scale" of 1 update check per cluster deployment. what's the approximate scale for the fuse update checker? we would need to be more careful if the scale suddenly is 100-1000 times larger.

Xenorith · 2023-09-06T23:30:57Z

dora/core/common/src/main/java/alluxio/util/UpdateCheckUtils.java

@@ -119,7 +114,6 @@ public static String getUserAgentString(String id, List<String> additionalInfo)
    List<String> info = new ArrayList<>();
    info.add(id);
    addUserAgentEnvironments(info);
-    addUserAgentFeatures(info);


is this removal on purpose? i do not see these user agent features readded for the master side update check

yes, this is intentional. original addUserAgentFeatures method is only covering master features and none of them is relevant after 300. Now, if we need any feature related info, make it in their respective UpdateChecker

apc999 · 2023-09-07T06:28:49Z

@LuQQiu @Xenorith PTAL

Xenorith

these changes look good to me, but i think it will be a good idea to update the endpoint's version to reflect the set of new changes. https://github.com/Alluxio/alluxio/blob/main/dora/core/common/src/main/java/alluxio/check/UpdateCheck.java#L83 is using a path with /v0 and i would recommend to change it to /v1. but i will also have to make the corresponding changes on the receiving side for this to properly work, so please hold off on merging until it is ready

Xenorith · 2023-09-15T10:08:41Z

@LuQQiu can i get a (fully populated) example of a http request to compare the server side with?

LuQQiu · 2023-10-02T18:19:32Z

@Xenorith @Zijian-Zhu
the others should be similar to before, but userAgent string is different

Alluxio/304-SNAPSHOT (59eea89a-8986-49d2-93b3-528d5e6243ef; OS:Mac OS X; ec2; localAlluxioMetadataCache; localKernelDataCache; UnderlyingFileSystem:s3; processType:FUSE)

User agent string contains ID, agent environments, processType similar to original user agent

  @VisibleForTesting
  public static String getUserAgentString(String id, CommonUtils.ProcessType processType,
      List<String> additionalInfo) {
    List<String> info = new ArrayList<>();
    info.add(id);
    addUserAgentEnvironments(info);
    info.addAll(additionalInfo);
    info.add(String.format("processType:%s", processType.toString()));
    return String.format("Alluxio/%s (%s)", ProjectConstants.VERSION,
        Joiner.on(USER_AGENT_SEPARATOR + " ").skipNulls().join(info));
  }

    if (fuseOptions.getFileSystemOptions().isDataCacheEnabled()) {
      mFuseInfo.add(LOCAL_ALLUXIO_DATA_CACHE);
    }
    if (fuseOptions.getFileSystemOptions().isMetadataCacheEnabled()) {
      mFuseInfo.add(LOCAL_ALLUXIO_METADATA_CACHE);
    }
    if (!fuseOptions.getFuseMountOptions().contains("direct_io")) {
      mFuseInfo.add(LOCAL_KERNEL_DATA_CACHE);
    }

UnderFileSystemType: "alluxio" or "local" or ufs type (e.g. "s3", "hdfs"

    mFuseInfo.add(String.format("UnderlyingFileSystem:%s", getUnderlyingFileSystem(fuseOptions)));
    if (!fuseOptions.getFileSystemOptions().getUfsFileSystemOptions().isPresent()) {
      return ALLUXIO_FS;
    }
    String ufsAddress = fuseOptions.getFileSystemOptions()
        .getUfsFileSystemOptions().get().getUfsAddress();
    if (URIUtils.isLocalFilesystem(ufsAddress)) {
      return LOCAL_FS;
    }
    String[] components = ufsAddress.split(":");
    if (components.length < 2) {
      return "unknown";
    }

Zijian-Zhu · 2023-10-23T17:26:15Z

the update for receiving side is done. Please update the endpoint from path /v0to /v1.

Xenorith · 2023-11-02T19:28:29Z

guessing the integration test failure is real:

2023-11-01T21:29:23.3531640Z 21:29:23.351 [ERROR] Tests run: 8, Failures: 2, Errors: 0, Skipped: 1, Time elapsed: 1.057 s <<< FAILURE! - in alluxio.fuse.meta.FuseUpdateCheckerTest
2023-11-01T21:29:23.3533855Z 21:29:23.352 [ERROR] alluxio.fuse.meta.FuseUpdateCheckerTest.UnderFileSystemAlluxio  Time elapsed: 0.01 s  <<< FAILURE!
2023-11-01T21:29:23.3534882Z java.lang.AssertionError
2023-11-01T21:29:23.3535407Z 	at org.junit.Assert.fail(Assert.java:87)
2023-11-01T21:29:23.3536037Z 	at org.junit.Assert.assertTrue(Assert.java:42)
2023-11-01T21:29:23.3536961Z 	at org.junit.Assert.assertTrue(Assert.java:53)
2023-11-01T21:29:23.3538059Z 	at alluxio.fuse.meta.FuseUpdateCheckerTest.UnderFileSystemAlluxio(FuseUpdateCheckerTest.java:37)
2023-11-01T21:29:23.3539200Z 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2023-11-01T21:29:23.3540370Z 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2023-11-01T21:29:23.3541553Z 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2023-11-01T21:29:23.3542531Z 	at java.lang.reflect.Method.invoke(Method.java:498)
2023-11-01T21:29:23.3544985Z 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
2023-11-01T21:29:23.3546584Z 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
2023-11-01T21:29:23.3548070Z 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
2023-11-01T21:29:23.3549438Z 	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
2023-11-01T21:29:23.3550535Z 	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
2023-11-01T21:29:23.3551606Z 	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
2023-11-01T21:29:23.3552686Z 	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
2023-11-01T21:29:23.3553776Z 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
2023-11-01T21:29:23.3555141Z 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
2023-11-01T21:29:23.3556150Z 	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
2023-11-01T21:29:23.3557021Z 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
2023-11-01T21:29:23.3557949Z 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
2023-11-01T21:29:23.3558872Z 	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
2023-11-01T21:29:23.3559788Z 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
2023-11-01T21:29:23.3560698Z 	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
2023-11-01T21:29:23.3561642Z 	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
2023-11-01T21:29:23.3562656Z 	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:316)
2023-11-01T21:29:23.3563929Z 	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:240)
2023-11-01T21:29:23.3565249Z 	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:214)
2023-11-01T21:29:23.3566685Z 	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:155)
2023-11-01T21:29:23.3568114Z 	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
2023-11-01T21:29:23.3570466Z 	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
2023-11-01T21:29:23.3571867Z 	at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
2023-11-01T21:29:23.3573303Z 	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)

@LuQQiu do you know what could be wrong?

Xenorith · 2023-11-07T23:23:44Z

alluxio-bot, merge this please

### What changes are proposed in this pull request? Improve UpdateChecker ### Why are the changes needed? 1. remove irrelevant master features 2. turn on fuse update check pr-link: Alluxio#18101 change-id: cid-8f6801fedda5f7710d111939e50cfd5f4372b7e5

Improve UpdateChecker

b42ddb8

apc999 requested review from dbw9580 and LuQQiu September 4, 2023 07:33

apc999 commented Sep 4, 2023

View reviewed changes

Further improvement

e2371ae

apc999 force-pushed the fuseUpdate branch from 73af81c to e2371ae Compare September 6, 2023 07:32

apc999 commented Sep 6, 2023

View reviewed changes

Xenorith reviewed Sep 6, 2023

View reviewed changes

Address comments

f7f5d16

Xenorith reviewed Sep 8, 2023

View reviewed changes

Xenorith force-pushed the main branch from dfa74af to 5fb603d Compare September 11, 2023 01:34

apc999 force-pushed the main branch 2 times, most recently from 2fec0ec to b597c61 Compare October 17, 2023 19:11

LuQQiu added 3 commits November 1, 2023 14:11

Before merging conflicts

cf40392

Resolve conflicts

ee0745a

Update endpoint version

ac1a690

Remove impossible test case and cleanup unusable value

c8cee25

Xenorith added the type-feature This issue is a feature request label Nov 7, 2023

Xenorith approved these changes Nov 7, 2023

View reviewed changes

alluxio-bot merged commit 246c7ee into Alluxio:main Nov 7, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve UpdateChecker #18101

Improve UpdateChecker #18101

apc999 commented Sep 4, 2023

apc999 Sep 4, 2023

dbw9580 Sep 4, 2023

apc999 Sep 4, 2023

LuQQiu Sep 5, 2023

Xenorith Sep 5, 2023

apc999 Sep 6, 2023

apc999 Sep 4, 2023

dbw9580 Sep 4, 2023

LuQQiu commented Sep 5, 2023

LuQQiu commented Sep 5, 2023

apc999 Sep 6, 2023

Xenorith left a comment

Xenorith Sep 6, 2023

apc999 Sep 7, 2023

apc999 commented Sep 7, 2023

Xenorith left a comment

Xenorith commented Sep 15, 2023

LuQQiu commented Oct 2, 2023

Zijian-Zhu commented Oct 23, 2023

Xenorith commented Nov 2, 2023

Xenorith commented Nov 7, 2023

Improve UpdateChecker #18101

Improve UpdateChecker #18101

Conversation

apc999 commented Sep 4, 2023

What changes are proposed in this pull request?

Why are the changes needed?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuQQiu commented Sep 5, 2023

LuQQiu commented Sep 5, 2023

Choose a reason for hiding this comment

Xenorith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apc999 commented Sep 7, 2023

Xenorith left a comment

Choose a reason for hiding this comment

Xenorith commented Sep 15, 2023

LuQQiu commented Oct 2, 2023

Zijian-Zhu commented Oct 23, 2023

Xenorith commented Nov 2, 2023

Xenorith commented Nov 7, 2023