Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix read file in jar 20190330 dq #638

Closed
wants to merge 9 commits into from

Conversation

dqing0
Copy link
Contributor

@dqing0 dqing0 commented Mar 30, 2019

Does this pull request fix one issue?

Fixes #634

Describe how you did it

1.参考FileRefreshableDataSource,FileWritableDataSource实现专门读取和写入jar文件的方法
2.通过jdk的JarFile,和jarEntry读取jar包中的文件
3.能够读取jar包中的文件
4.jar包中的文件修改可以更新到内存
5.在控制台更改规则后,能修改本地jar文件中的值

为什么没有通过添加构造器的方法去获取呢?
因为jar文件和普通的文件的读取和写有一些的区别,特别是写,还有classLoader.getResourceAsStream()方法只能获取inputstream对象,只能读取内容,但是它的更新时间获取不到,会导致修改配置文件不能更新规则

Describe how to verify it

1.运行JarFileDataSourceDemo,JarFileDataSourceInit 中的main方法
2.读取文件内容,修改jar包中的配置文件会更新到内存,在控制台更改规则能更新到本地jar包中的配置文件,
这些功能已验证

Special notes for reviews

1.写jar包中的某个文件时,其他不用变更的文件也要重写一遍,这点很坑,但jdk提供的JarFile好像只能
这样做,不知道有没有更好的方法取重写jar中的文件

@CLAassistant
Copy link

CLAassistant commented Mar 30, 2019

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

Codecov Report

Merging #638 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #638   +/-   ##
=========================================
  Coverage     40.06%   40.06%           
  Complexity     1292     1292           
=========================================
  Files           284      284           
  Lines          8916     8916           
  Branches       1192     1192           
=========================================
  Hits           3572     3572           
  Misses         4886     4886           
  Partials        458      458

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 015c87c...3623605. Read the comment docs.

@sczyh30 sczyh30 added the to-review To review label Mar 31, 2019
@sczyh30
Copy link
Member

sczyh30 commented Apr 1, 2019

Hi, thanks for contributing. One more thing to discuss: is it really necessary to modify the config file inside the jar? I don't think it makes sense to modify files inside the jar because once the jar is upgraded, the files would be lost. Maybe writing to outside files (current implementation) is more appropriate.

What do you think of it? @CarpenterLee @fangjian0423

@dqing0
Copy link
Contributor Author

dqing0 commented Apr 1, 2019

now,I temporarily put the configuration outside the jar to sovle it like you said. this way just ,for those who want to configure in jar. because springboot recommended use jar to run app. so if there is a better choice,i choose use zk or dubbo .., but if you don't want to depend ohthers , you can only use
it

@dqing0
Copy link
Contributor Author

dqing0 commented Apr 1, 2019

And ,According to the original method, if the jar is upgraded, the files also would be lost ,because
the configuration is in target classes

@fangjian0423
Copy link
Contributor

Sorry for the late reply.

I agree with @sczyh30, outside file is better than file in jar.

Sorry for the first time I said adding a new constructor in FileRefreshableDataSource because check outside file modified is different in jar. Create new datasource is better.

@dqing0 dqing0 closed this Apr 2, 2019
@sczyh30
Copy link
Member

sczyh30 commented Apr 2, 2019

IMHO, it makes no sense for dynamic file in jar as it's not expected to be changed (users cannot modify the file in jar manually). But reading config from the file in jar is okay. So just a FileInJarReadableDataSource might be enough (don't need to refresh).

@dqing0
Copy link
Contributor Author

dqing0 commented Apr 2, 2019

IMHO, it makes no sense for dynamic file in jar as it's not expected to be changed (users cannot modify the file in jar manually). But reading config from the file in jar is okay. So just a FileInJarReadableDataSource might be enough (don't need to refresh).

so,i change my pullrequest,only keep FileInJarReadableDataSource ?

@sczyh30
Copy link
Member

sczyh30 commented Apr 2, 2019

IMHO, it makes no sense for dynamic file in jar as it's not expected to be changed (users cannot modify the file in jar manually). But reading config from the file in jar is okay. So just a FileInJarReadableDataSource might be enough (don't need to refresh).

so,i change my pullrequest,only keep FileInJarReadableDataSource ?

Feel free to do that :)
We can assume that the config file in jar won't be changed, so it don't need to be refreshable.

@dqing0
Copy link
Contributor Author

dqing0 commented Apr 2, 2019

We can assume that the config file in jar won't be changed, so it don't need to be refreshable.

ok,i change the pr

@sczyh30 sczyh30 removed the to-review To review label Apr 2, 2019
CST11021 pushed a commit to CST11021/Sentinel that referenced this pull request Nov 3, 2021
* prepare to separate production-ready projects from the external projects

* Update fastjson to the latest stable version

* Clean code, don't list the default config in JVM

* Update README.md

* update the year info in NOTICE

* Release semaphore when timeout

* [ISSUE#525]add aclClient PRCHook for message track

* [ISSUE#525]add the apache license text,remove the merged from master branch

* [ISSUE#525]add aclClient PRCHook for message track,remove the merged content of notice and readme.md from master branch,add some unit test to increase the code coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FileWritableDataSource :file:/xx/xx.jar!/BOOT-INF/classes!/FlowRule.json (no such file or directory)
5 participants