SDK-321: Optimizing SDK Cache Management #255

wikumChamith · 2023-09-20T09:09:34Z

Description of what I changed

Add the enableDataSaving parameter to the setup-sdk plugin. This parameter enables the data-saving mode. When using this mode, the SDK will reuse the existing cache folder rather than creating a new one.
In normal mode, SDK will remove the cache folder after running the setup.

Issue I worked on

see https://issues.openmrs.org/browse/SDK-321

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean install right before creating this pull request and
added all formatting changes to my commit.

No? -> execute the above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

Add the enableDataSaving parameter to the setup-sdk plugin. This parameter enables the data-saving mode. When using this mode, the SDK will reuse the existing cache folder rather than creating a new one. In normal mode, SDK will remove the cache folder after running the setup.

dkayiwa · 2023-09-24T09:21:35Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/SetupSDK.java


 @Mojo(name = "setup-sdk", requiresProject = false)
 public class SetupSDK extends AbstractTask {

+	@Parameter(defaultValue = "false", property = "enableDataSaving")
+	private boolean enableDataSaving;


I feel like we should change the above name to something that reflects what exactly the effects of this flag will be on the actual implementation.

dkayiwa · 2023-09-24T09:30:25Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/NodeHelper.java

+			boolean isDataSavingMode = Boolean.parseBoolean(sdkProperties.getProperty("enableDataSaving"));
+			String cacheName = "openmrs-sdk-node";
+			if (isDataSavingMode) {
+				logger.info("OpenMRS SDK is running on data saving mode");


I would not call this a data saving mode because it may be used for other reasons like performance instead of just saving data.

dkayiwa · 2023-09-25T11:23:43Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/SetupSDK.java


 @Mojo(name = "setup-sdk", requiresProject = false)
 public class SetupSDK extends AbstractTask {

+	@Parameter(defaultValue = "true", property = "removeCacheAfterSetup")
+	private boolean removeCacheAfterSetup;


Can you add some documentation to this parameter?

dkayiwa · 2023-09-25T11:25:22Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/NodeHelper.java

+		File tempDir = new File(tempDirPath);
+		if(tempDir.exists() && tempDir.isDirectory()) {
+			File[] cache = tempDir.listFiles(file -> file.isDirectory() && file.getName().startsWith("openmrs-sdk-node"));
+			if(cache == null) return null;


https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-CodeStyle

pom.xml

dkayiwa · 2023-09-25T11:29:09Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/NodeHelper.java

+			boolean isRemoveCacheAfterSetup = Boolean.parseBoolean(sdkProperties.getProperty("removeCacheAfterSetup"));
+			String cacheName = "openmrs-sdk-node";
+			if (!isRemoveCacheAfterSetup) {
+				logger.info("OpenMRS SDK will reuse the NPM cache. Run openmrs-sdk:setup-sdk -DremoveCacheAfterSetup=true to disable it.");


Can we be consistent? The description says reuse npm cache but the method name says remove cache.

Should we rename the parameter as "ReuseNpmCache" ??

What do you think is better?

I think logger info is consistent with implementation. If isRemoveCacheAfterSetup is false the NPM will reuse NPM caches without deleting them.

How about something along the lines of reuseNodeCache?

sounds good

I changed the parameter name

dkayiwa · 2023-09-25T13:45:10Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/NodeHelper.java

+			if (cache == null) {
+				return null;
+			}
+			if (cache.length > 0) {


What happens to the rest of the caches?

Should we delete them??

If they are more than one, will the first one (at index 0) be consistently the same for subsequent calls?

@dkayiwa what do you suggest we should do here? We could delete the other caches.

yes

This actually isn't guaranteed. We're operating on files in the order returned from listFiles() which has this caveat:

"There is no guarantee that the name strings in the resulting array will appear in any specific order; they are not, in particular, guaranteed to appear in alphabetical order. "

Similarly, createTempDirectory() says:

"The details as to how the name of the directory is constructed is implementation dependent and therefore not specified."

In order to guarantee that this always returns the same folder, we'd need a guarantee that listFiles() returned files in a consistent order and that createTempDirectory() generates files in a defined order that will fit the order returned by listFiles(), but those details are specifically disclaimed.

(On Windows, the default JVM does indeed use the tick count as the source of randomness for createTempDirectory() and listFiles() likely returns in a consistent order, which would likely result in this appearing to work; however, Linux, the underlying mkstemp() implementation will try to use a RNG, falling back to ticks if not available on that device. I haven't looked into the details of macOS.)

Probably the thing to do is to make the tempDir a class variable instead of an instance variable so it's only set once per JVM, which would guarantee consistent results across multiple invocations of the NodeHelper within a single invocation of the SDK.

On reflection, actually, I think a better strategy is: if the user wants to use a consistent NPM cache directory, use a named folder in the directory the SDK "servers directory" (defined here). Just name it something unlikely to be a server name, e.g., _openmrs_sdk_node_cache. That's cleaner, easier for end-users to understand and nobody has to go poking around for the right "openmrs-sdk-node.1234567" folder.

dkayiwa · 2023-09-25T13:47:08Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/SetupSDK.java

+	 * By default, this is set to 'true' to save storage space.
+	 * If set to 'false', the SDK will reuse the existing NPM cache
+	 */
+	@Parameter(defaultValue = "true", property = "removeCacheAfterSetup")


Shouldn't the default be what it was before these changes?

I changed the default value and the code to reflect the property name.

maven-plugin/src/main/java/org/openmrs/maven/plugins/SetupSDK.java

ibacher · 2023-09-29T14:59:50Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/NodeHelper.java

+
+	private static final Logger logger = LoggerFactory.getLogger(NodeHelper.class);
+
+	public Path getTempDir() {


Does this need to be public? Also, coding style, accessor properties should always come after constructors.

ibacher · 2023-09-29T15:00:30Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/NodeHelper.java

 		this.mavenProject = mavenProject;
 		this.mavenSession = mavenSession;
 		this.pluginManager = pluginManager;
 		try {
-			this.tempDir = Files.createTempDirectory("openmrs-sdk-node");
-			this.tempDir.toFile().deleteOnExit();
+			Properties sdkProperties = getSdkProperties();


It's preferrable, I think to move this code into getTempDir() that way we can defer creating a temporary directory until we actually need it.

ibacher · 2023-09-29T15:00:47Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/NodeHelper.java

-			this.tempDir.toFile().deleteOnExit();
+			Properties sdkProperties = getSdkProperties();
+			boolean reuseNodeCache = Boolean.parseBoolean(sdkProperties.getProperty("reuseNodeCache"));
+			String cacheName = "openmrs-sdk-node";


Seems like this should just be a class constant, no?

ibacher · 2023-09-29T15:22:46Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/NodeHelper.java

+			if (cache == null) {
+				return null;
+			}
+			if (cache.length > 0) {


yes

This actually isn't guaranteed. We're operating on files in the order returned from listFiles() which has this caveat:

"There is no guarantee that the name strings in the resulting array will appear in any specific order; they are not, in particular, guaranteed to appear in alphabetical order. "

Similarly, createTempDirectory() says:

"The details as to how the name of the directory is constructed is implementation dependent and therefore not specified."

In order to guarantee that this always returns the same folder, we'd need a guarantee that listFiles() returned files in a consistent order and that createTempDirectory() generates files in a defined order that will fit the order returned by listFiles(), but those details are specifically disclaimed.

(On Windows, the default JVM does indeed use the tick count as the source of randomness for createTempDirectory() and listFiles() likely returns in a consistent order, which would likely result in this appearing to work; however, Linux, the underlying mkstemp() implementation will try to use a RNG, falling back to ticks if not available on that device. I haven't looked into the details of macOS.)

Probably the thing to do is to make the tempDir a class variable instead of an instance variable so it's only set once per JVM, which would guarantee consistent results across multiple invocations of the NodeHelper within a single invocation of the SDK.

wikumChamith · 2023-09-30T08:43:01Z

@ibacher I made the changes you requested.

ibacher · 2023-10-03T18:48:21Z

@wikumChamith I think what I actually asked for was this:

On reflection, actually, I think a better strategy is: if the user wants to use a consistent NPM cache directory, use a named folder in the directory the SDK "servers directory" (defined here). Just name it something unlikely to be a server name, e.g., _openmrs_sdk_node_cache. That's cleaner, easier for end-users to understand and nobody has to go poking around for the right "openmrs-sdk-node.1234567" folder.

Also, even if we have a global option for this (which is fine), it would be nice to have a local option to override the global option without needing to run setup-sdk all over again (especially as that has side-effects I'd rather avoid).

Add the enableDataSaving parameter to the setup-sdk plugin. This parameter enables the data-saving mode. When using this mode, the SDK will reuse the existing cache folder rather than creating a new one. In normal mode, SDK will remove the cache folder after running the setup.

wikumChamith · 2023-10-05T15:27:54Z

@ibacher I made the changes you suggested.

ibacher

Thanks @wikumChamith! There are two more places we should be able to override the node cache value:

BuildDistro
ServerUpgrader

Then I think this will be all set. Thanks for the work on this!

wikumChamith · 2023-10-12T03:21:35Z

@ibacher I integrated the cache management to build-distro and server upgrader.

makombe · 2023-10-15T09:06:23Z

S3yg

…

On Wed, 20 Sep 2023, 12:09 Wikum Weerakutti, ***@***.***> wrote: Description of what I changed - Add the enableDataSaving parameter to the setup-sdk plugin. This parameter enables the data-saving mode. When using this mode, the SDK will reuse the existing cache folder rather than creating a new one. - In normal mode, SDK will remove the cache folder after running the setup. Issue I worked on see https://issues.openmrs.org/browse/SDK-321 Checklist: I completed these to help reviewers :) - My IDE is configured to follow the *code style* <https://wiki.openmrs.org/display/docs/Java+Conventions> of this project. No? Unsure? -> configure your IDE <https://wiki.openmrs.org/display/docs/How-To+Setup+And+Use+Your+IDE>, format the code and add the changes with git add . && git commit --amend - I have *added tests* to cover my changes. (If you refactored existing code that was well tested you do not have to add tests) No? -> write tests and add them to this commit git add . && git commit --amend - I ran mvn clean install right before creating this pull request and added all formatting changes to my commit. No? -> execute the above command - All new and existing *tests passed*. No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works. - My pull request is *based on the latest changes* of the master branch. No? Unsure? -> execute command git pull --rebase upstream master ------------------------------ You can view, comment on, or merge this pull request online at: #255 Commit Summary - 9e2a1dd <9e2a1dd> SDK-321: Optimizing SDK Cache Management File Changes (5 files <https://github.com/openmrs/openmrs-sdk/pull/255/files>) - *M* maven-plugin/src/main/java/org/openmrs/maven/plugins/SetupSDK.java <https://github.com/openmrs/openmrs-sdk/pull/255/files#diff-20401bf5b076fe51b23973f37b44022b5acff19d4cde389befa6e56c7e68a81a> (12) - *M* maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/NodeHelper.java <https://github.com/openmrs/openmrs-sdk/pull/255/files#diff-5d26fc863281dda12d77af873dc4e40e83ad5f98daf37ce8ef292da97dabc3dc> (48) - *M* maven-plugin/src/main/java/org/openmrs/maven/plugins/utility/SpaInstaller.java <https://github.com/openmrs/openmrs-sdk/pull/255/files#diff-e90c9e5e322d90165e0236bb5ea86ea79a8fdc53a9d374c4291418bc95982fb8> (19) - *M* pom.xml <https://github.com/openmrs/openmrs-sdk/pull/255/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8> (7) - *M* sdk-commons/src/main/java/org/openmrs/maven/plugins/utility/PropertiesUtils.java <https://github.com/openmrs/openmrs-sdk/pull/255/files#diff-1521ea06c40e43b7e609c6a2f57bb03e2fdbba3264bcb9550475eea60f29d11a> (39) Patch Links: - https://github.com/openmrs/openmrs-sdk/pull/255.patch - https://github.com/openmrs/openmrs-sdk/pull/255.diff — Reply to this email directly, view it on GitHub <#255>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB2CWVNLCSLULXFNPRGJPDX3KXF5ANCNFSM6AAAAAA47SIGLQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

ibacher · 2023-10-16T00:15:19Z

sdk-commons/src/main/java/org/openmrs/maven/plugins/utility/PropertiesUtils.java

@@ -33,6 +36,7 @@
 public class PropertiesUtils {

 	private static final Logger log = LoggerFactory.getLogger(PropertiesUtils.class);
+	private static final String SDK_PROPERTIES_FILE = "SDK Properties file";


Shouldn't this just be something like sdk.properties?

When writing the SDK properties to the files using Properties#store(java.io.OutputStream, java.lang.String) we have to pass a comment. This comment is used as the heading of the file. I renamed the variable name to eliminate any confusion. The actual file name of the properties file is "sdk.properties".

Then the constant name would be less confusing if it reflected that in one way or another like SDK_PROPERTIES_FILE_HEADER. Or just get rid of the constant because after all, you use it in only one place.

@dkayiwa I removed the variable in this PR: #257

dkayiwa · 2023-10-16T09:37:14Z

sdk-commons/src/main/java/org/openmrs/maven/plugins/utility/PropertiesUtils.java

+	public static void savePropertiesChangesToFile(Properties properties, File file)
+			throws MojoExecutionException {
+		try (OutputStream fos = new FileOutputStream(file)) {
+			properties.store(fos, SDK_PROPERTIES_FILE + ":");


What is the use of the full colon at the end of this comment?

I removed the colon.

wikumChamith force-pushed the SDK-321 branch from 9e2a1dd to 7ac1f6b Compare September 20, 2023 13:34

wikumChamith force-pushed the SDK-321 branch from 05f3b84 to 703adaf Compare September 21, 2023 02:49

dkayiwa reviewed Sep 24, 2023

View reviewed changes

wikumChamith force-pushed the SDK-321 branch from 703adaf to da1a394 Compare September 25, 2023 05:55

dkayiwa reviewed Sep 25, 2023

View reviewed changes

pom.xml Show resolved Hide resolved

dkayiwa reviewed Sep 25, 2023

View reviewed changes

wikumChamith force-pushed the SDK-321 branch from da1a394 to c6bdbbd Compare September 25, 2023 13:22

dkayiwa reviewed Sep 25, 2023

View reviewed changes

wikumChamith force-pushed the SDK-321 branch from c6bdbbd to 64b2faf Compare September 27, 2023 18:31

ibacher reviewed Sep 29, 2023

View reviewed changes

wikumChamith force-pushed the SDK-321 branch from 64b2faf to 6ad7585 Compare September 30, 2023 08:42

wikumChamith force-pushed the SDK-321 branch from 6ad7585 to edaf5e5 Compare October 5, 2023 14:50

wikumChamith force-pushed the SDK-321 branch from edaf5e5 to c7ee7a9 Compare October 5, 2023 15:41

wikumChamith requested review from dkayiwa and ibacher October 10, 2023 11:19

ibacher reviewed Oct 10, 2023

View reviewed changes

wikumChamith force-pushed the SDK-321 branch 2 times, most recently from 7f1f3a0 to 96fef6d Compare October 12, 2023 02:37

ibacher approved these changes Oct 14, 2023

View reviewed changes

ibacher reviewed Oct 16, 2023

View reviewed changes

dkayiwa reviewed Oct 16, 2023

View reviewed changes

SDK-321: Optimizing SDK Cache Management

ae636ec

wikumChamith force-pushed the SDK-321 branch from 96fef6d to ae636ec Compare October 16, 2023 10:21

dkayiwa merged commit 2623cd9 into openmrs:master Oct 16, 2023
2 checks passed

wikumChamith mentioned this pull request Oct 16, 2023

Removing SDK_PROPERTIES_COMMENT variable #257

Merged

5 tasks


		private static final Logger logger = LoggerFactory.getLogger(NodeHelper.class);

		public Path getTempDir() {

SDK-321: Optimizing SDK Cache Management #255

SDK-321: Optimizing SDK Cache Management #255

Conversation

wikumChamith commented Sep 20, 2023

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

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

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

dkayiwa Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wikumChamith Sep 28, 2023 • edited Loading

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

Choose a reason for hiding this comment

wikumChamith commented Sep 30, 2023 • edited Loading

ibacher commented Oct 3, 2023

wikumChamith commented Oct 5, 2023

ibacher left a comment

Choose a reason for hiding this comment

wikumChamith commented Oct 12, 2023

makombe commented Oct 15, 2023 via email

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

dkayiwa Sep 25, 2023 •

edited

Loading

wikumChamith Sep 28, 2023 •

edited

Loading

wikumChamith commented Sep 30, 2023 •

edited

Loading